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

Some refactoring of z_order fuctionality #303

Merged
merged 15 commits into from
Dec 8, 2023
Merged

Some refactoring of z_order fuctionality #303

merged 15 commits into from
Dec 8, 2023

Conversation

teutoburg
Copy link
Contributor

@teutoburg teutoburg commented Dec 5, 2023

After a more general discussion on the topic with @astronomyk and @hugobuddel, the effects should not be ordered according to the final two digits of their z-orders, but rather the order should follow that of definition in the respective IRDB yaml files (which is the current behavior). Ultimately, the z_order might be removed completely, so it's not worth investing much if any time in any functionality related to it.

However, that discussion came after the commits found in this PR (except the final one), and everything else can still be considered and improvement, even if it's only for the docstrings, so I'm still in favor of merging. The only test that now fails is the codecov, but creating new tests for something that will likely be removed soon anyway, seems unnecessary, so I'd just ignore that.

@teutoburg teutoburg self-assigned this Dec 5, 2023
@teutoburg teutoburg added refactor Implementation improvement tests Related to unit or integration tests effects Related to a ScopeSim effect labels Dec 5, 2023
Copy link

codecov bot commented Dec 6, 2023

Codecov Report

Attention: 19 lines in your changes are missing coverage. Please review.

Comparison is base (215d86d) 80.04% compared to head (4e0016c) 80.07%.
Report is 5 commits behind head on dev_master.

Files Patch % Lines
scopesim/optics/optical_element.py 48.27% 15 Missing ⚠️
scopesim/effects/effects_utils.py 71.42% 2 Missing ⚠️
scopesim/optics/optics_manager.py 81.81% 2 Missing ⚠️
Additional details and impacted files
@@              Coverage Diff               @@
##           dev_master     #303      +/-   ##
==============================================
+ Coverage       80.04%   80.07%   +0.03%     
==============================================
  Files             145      147       +2     
  Lines           14771    14819      +48     
==============================================
+ Hits            11823    11866      +43     
- Misses           2948     2953       +5     

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

@teutoburg teutoburg changed the title Ensure correct order off effects Some refactoring of z_order fuctionality Dec 7, 2023
@teutoburg teutoburg removed the effects Related to a ScopeSim effect label Dec 7, 2023
@teutoburg teutoburg marked this pull request as ready for review December 7, 2023 18:19
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.

Clearly better!

@teutoburg teutoburg merged commit 262e894 into dev_master Dec 8, 2023
21 of 22 checks passed
@teutoburg teutoburg deleted the fh/zorder branch December 8, 2023 13:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Implementation improvement tests Related to unit or integration tests
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants