Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove obsolete files #340

Merged
merged 1 commit into from
Jan 15, 2024
Merged

Remove obsolete files #340

merged 1 commit into from
Jan 15, 2024

Conversation

teutoburg
Copy link
Contributor

Created a tag on dev_master just before this branch-off, so should we ever need anything from those old files, we can just checkout that.

Also deleted MANIFEST.in, which is now obsolete (I think) and new_features.rst, which was hopelessly outdated anyway (use CHANGLOG.md in the future for this...).

Any reservations against this move?

Copy link

codecov bot commented Jan 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (d97dfaa) 77.04% compared to head (d1ecb3f) 77.04%.

Additional details and impacted files
@@             Coverage Diff             @@
##           dev_master     #340   +/-   ##
===========================================
  Coverage       77.04%   77.04%           
===========================================
  Files              57       57           
  Lines            7693     7693           
===========================================
  Hits             5927     5927           
  Misses           1766     1766           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@teutoburg teutoburg marked this pull request as ready for review January 15, 2024 17:02
Copy link
Collaborator

@hugobuddel hugobuddel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm I already wrote a comment here, where I included the commits that introduced these OLD files. As in, that might make it easier to figure out whether there is something in them worth keeping. But can't find that comment.

Anyway, it was still to complicated to figure out whether there was still something worthwhile in there.. You'd need to ask Kieran I think.

Personally I'm in favor of removing this old code.

@teutoburg teutoburg merged commit 3d174ef into dev_master Jan 15, 2024
16 checks passed
@teutoburg teutoburg deleted the fh/removeoldstuff branch January 15, 2024 21:25
@hugobuddel
Copy link
Collaborator

In AstarVienna/speXtra#13 you grumbled about coverage going down by disabling coverage on test files. Then I realized that coverage should have gone up here, because we are removing uncovered files, but it didn't.

Apparently we don't count untested files in our coverage. Perhaps we should count all not-explicitly excluded Python files? Or maybe have a test that simply tries to import all Python files?

I looked around a bit, and so far found one other: https://github.com/AstarVienna/ScopeSim/blob/dev_master/scopesim/detector/nghxrg.py . The only place it was used was, you might have guessed:

$ rg nghxrg
ScopeSim/OLD_code/OLD_detector.py
106:from scopesim.detector.nghxrg import HXRGNoise

And indeed, it is broken:

python -c "from scopesim.detector.nghxrg import HXRGNoise"
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "[...]/scopesim/detector/__init__.py", line 1, in <module>
    from .detector import Detector
  File "[...]/scopesim/detector/detector.py", line 5, in <module>
    from ..optics import image_plane_utils as imp_utils
  File "[...]/scopesim/optics/__init__.py", line 5, in <module>
    from .optical_train import OpticalTrain
  File "[...]/scopesim/optics/optical_train.py", line 19, in <module>
    from ..detector import DetectorArray
ImportError: cannot import name 'DetectorArray' from partially initialized module 'scopesim.detector' (most likely due to a circular import) ([...]/scopesim/detector/__init__.py)

Maybe I'll give it a quick shot to try to write a test to import all Python modules. Not sure what to do with nghxrg.py though, maybe it would still be useful.

@teutoburg
Copy link
Contributor Author

Not sure what to do with nghxrg.py though

Specifically about this ngxjsiejrpokjfklkarklmcpy.py, I mean nghxrg.py, according to @astronomyk this is "the best thing we have" to simulate detector noise, but it's currently not used anywhere for performance reasons. Hence we keep it around for the time being. That's also why I didn't spend any time (yet) to refactor or lint that module, as you'll see when you look at it...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants