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

Allow for an artificial horizon when generating an *Observer sky map. #25

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

David-McKenna
Copy link
Contributor

This adds a kwarg to BaseObserver.generate() to allow for the horizon to be set at above 0 degrees, allowing for only a fraction of the visible sky to be sampled.

Additionally, I have added tests to ensure that only a fraction of the sky is visible, and that the rad/deg convention used by pyephem for the lon/lat values is also respected for the elevation keyword.

Copy link

what-the-diff bot commented Apr 30, 2024

PR Summary

  • New attribute added to base_observer
    The base_observer.py file now includes a new attribute, _horizon_elevation. Additionally, the generate method in this file can now accept an argument for horizon_elevation. This allows elevation information to be taken into consideration when observers generate sky data. There's also a new feature that if horizon_elevation is less than 0, a ValueError is logged.

  • Improvements to GSMObserver16 testing
    Test coverage for the GSMObserver16 class in test_gsm2016.py has been extended. Test instances now attribute different values for horizon_elevation. We also introduced a new assertions step to compare variables d_20deg_horizon and d_20deg2rad_horizon.

  • Extensive testing parameters for horizon elevation in gsm_observer
    In test_gsm_observer.py, there's a new test case addressing scenarios when horizon_elevation is less than 0. This addition enhances the robustness of our codebase by validating edge cases. There is also an expanded list of arguments for the generate method which now tests possible values for horizon_elevation.

  • Enhanced functionalities of the LFSMObserver class
    Test_lfsm.py now includes two new test cases for LFSMObserver class and expanded testing for horizon_elevation parameter. We've introduced similar variables d_20deg_horizon and d_20deg2rad_horizon and corresponding assertion steps for comparison as those added in GSMObserver16.

Overall, this PR provides an important update to sky observation functionalities, allowing for an elevation parameter to be considered, and extends test coverage, making our codebase more robust.

@David-McKenna
Copy link
Contributor Author

Platform dependent masking fraction? Oh, boy, that's going to be fun to figure out...

@David-McKenna David-McKenna marked this pull request as draft April 30, 2024 09:13
@telegraphic telegraphic added the enhancement New feature or request label May 6, 2024
pygdsm/base_observer.py Outdated Show resolved Hide resolved
@telegraphic
Copy link
Owner

LGTM! FWIW, I would be happy to merge w/o a perfect test as the masking fraction issue is very strange. I've just updated the codecov CODECOV_TOKEN secret, so if you merge from master it should pass tests. (Also happy to merge w/o that test passing).

@David-McKenna
Copy link
Contributor Author

I'd like to find some way of making it less flakey -- whether it's by using isclose like you mentioned the other day, or by finding a value that reduces the noise/jitter between platforms (i.e., a high elevation where there is a lower fraction of masked pixels).

I'll hopefully have time to find some kind of resolution for this later this week.

I seem to have run into a case of -ffast-math optimizations differing by platform, resulting in the tests returning different values on my ARM64 laptop, vs the x86_64 runners. Increasing the elevation reduces the effective number of boundary pixels, reducing the chance of the optimizations resulting in a variable number of pixels being masked by platform.
@David-McKenna
Copy link
Contributor Author

David-McKenna commented May 9, 2024

While watching the checks execute I thought I might as well highlight that while you're linting with flake8, the errors are actually being ignored by the workflow, I'll see if I can patch that up in a bit.

Looks like that's intentional, https://github.com/telegraphic/pygdsm/blob/master/.github/workflows/python-app.yml#L37

@David-McKenna David-McKenna marked this pull request as ready for review May 9, 2024 04:54
@David-McKenna
Copy link
Contributor Author

Looks like CodeCov is out of action again, but the tests were successful this time

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants