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

Refactor some rarely-used utils functions #382

Merged
merged 2 commits into from
Mar 1, 2024
Merged

Conversation

teutoburg
Copy link
Contributor

@teutoburg teutoburg commented Mar 1, 2024

Also delete some code that's been commented out for ages.

A continuation of #381, I moved two functions from utils to surface_utils, because they were only used there anyway. Although it might be questionable if we need some of those at all...

Rant

I don't like all these return None at all. I think a function should always return the same type if at all possible. The return None are not new though, they're just a refactored ("return early") approach of what was already there. I think those return None cases should either raise an exception that's caught upstream, or not exist at all. But I kept them in for now, as something somewhere might depend on that None being returned (I hope not). Once we finally get around to doing some static type checking, we should find all these return None things thrown in our faces anyway...

Also delete some code that's been commented out for ages.
Copy link

codecov bot commented Mar 1, 2024

Codecov Report

Attention: Patch coverage is 87.27273% with 7 lines in your changes are missing coverage. Please review.

Project coverage is 74.92%. Comparing base (53ed24c) to head (d20a45d).

Files Patch % Lines
scopesim/optics/surface_utils.py 90.47% 4 Missing ⚠️
scopesim/utils.py 75.00% 2 Missing ⚠️
scopesim/optics/surface.py 80.00% 1 Missing ⚠️
Additional details and impacted files
@@              Coverage Diff               @@
##           dev_master     #382      +/-   ##
==============================================
- Coverage       74.94%   74.92%   -0.02%     
==============================================
  Files              56       56              
  Lines            7770     7768       -2     
==============================================
- Hits             5823     5820       -3     
- Misses           1947     1948       +1     

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

@teutoburg teutoburg added the refactor Implementation improvement label Mar 1, 2024
@teutoburg teutoburg self-assigned this Mar 1, 2024
@teutoburg teutoburg marked this pull request as ready for review March 1, 2024 13:13
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.

Approved.

Nevertheless,

  • Veering a bit close to developer wank 😋.
  • Returning None can be fine. We can type the functions with -> Optional[whatever]. At any rate, returning None would be better than raising an exception in the case of emission() etc. The only other option would be to return a SourceSpectrum with all zeros; I'd also be fine with that. Or maybe I'm misinterpreting what these Nones are doing; that could very well be the case.
  • Deleting the commented out code is fine with me. But that code is useful, and is not properly represented elsewhere. E.g. the METIS_Simulations do not have a BUNIT. But it doesn't seem to difficult to recreate this code if necessary. And indeed it is 5 years old.
  • I'd be fine with keeping the utility functions where they were, as they are not specific to surfaces. But I also wondered why they are necessary at all. I recall they are currently needed though.

All in all, it does make the code base cleaner.

@teutoburg teutoburg merged commit 2d57b6f into dev_master Mar 1, 2024
25 checks passed
@teutoburg teutoburg deleted the fh/somerefac branch March 1, 2024 14:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Implementation improvement
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

2 participants