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

Issue 1165: Checking datasets for MObs and Conforming w/ New Limb Darkening Updates #1217

Merged
merged 18 commits into from
Aug 11, 2023

Conversation

tamimfatahi
Copy link
Collaborator

@tamimfatahi tamimfatahi commented Aug 8, 2023

  • Added testing suite for ld.py
  • Modified EXOTIC to handle updated changes of ld.py for standard filters
  • If user is using MicroObservatory datasets, MObs CV filter will be used as default
  • Organized code

Two things I'd like to fix before making this a PR are the following:

  1. I'm currently working on getting standard filters with differing FWHM (ex: CV) to work properly. I should have this done soonish.
  2. I don't agree with the logic of the following in ld.py:
    "Clear (unfiltered) reduced to V sequence": "MObs CV"
    It doesn't make sense since not all clear filters are MicroObservatory ones. Maybe we remove this? Although, the description would be nice to keep to describe a non-MObs CV filter on the results page if we could keep it.

Closes #1165

- Modified EXOTIC to handle updated changes of ld.py for standard filters
- If user is using MicroObservatory datasets, MObs CV filter will be used as default
- Organized code
@tamimfatahi tamimfatahi self-assigned this Aug 8, 2023
@tamimfatahi tamimfatahi linked an issue Aug 8, 2023 that may be closed by this pull request
exotic/api/ld.py Show resolved Hide resolved
Copy link
Collaborator

@jpl-jengelke jpl-jengelke left a comment

Choose a reason for hiding this comment

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

Please move tests to the tests directory at the top level of the application. We can explore PyUnit to run them as an add-on later.

exotic/api/tests/test_ld.py Outdated Show resolved Hide resolved
@jpl-jengelke
Copy link
Collaborator

@rzellem Please comment/approve or add insight to the following ...

I don't agree with the logic of the following in ld.py:
"Clear (unfiltered) reduced to V sequence": "MObs CV"
It doesn't make sense since not all clear filters are MicroObservatory ones. Maybe we remove this? Although, the description would be nice to keep to describe a non-MObs CV filter on the results page if we could keep it.

As background, that is an alias to the MObs CV filter. Obviously we are using MObs CV as a default for clear in most cases. I think maybe @tamimfatahi is objecting to the generic wording being applied to a specific type of clear filter.

As an aside, if CV is entered and the fwhm vals don't match up to those for MObs CV do we still call it Custom with an abbreviation code of N/A? I believe AAVSO requested that all custom filters be tagged with 'N/A'.

@tamimfatahi may have more prescient concerns. Thanks.

@tamimfatahi tamimfatahi linked an issue Aug 9, 2023 that may be closed by this pull request
@tamimfatahi tamimfatahi removed a link to an issue Aug 9, 2023
@tamimfatahi tamimfatahi linked an issue Aug 9, 2023 that may be closed by this pull request
…ers due to AAVSO listing it as so. Added both filters to filters.py with null FWHM for searching (may not be in final version).
@tamimfatahi
Copy link
Collaborator Author

With these changes, the MObs CV filter will no longer be linked to CV. The good news is that users will not have to change anything as we will detect if their images came from MicroObservatory, so it won't matter what they list as their filter (as long as we can tell based on their header).

@jpl-jengelke
Copy link
Collaborator

Just as an aside, this is important for the TESS processing because precision is more critical given the nature of space-based telescope observations.

@jpl-jengelke
Copy link
Collaborator

jpl-jengelke commented Aug 9, 2023

I have some ideas and was considering pseudocode right here, but I think we would be more proficient doing it together again tonight. Generally, if we want to preserve user descriptions for certain filter 'name' (e.g. abbreviation) values, I think we should consider that after all the matching, and on very limited bases for the reason mentioned above, wavelengths should generally match filter descriptions.

Bear with me until we can talk about this shortly...

exotic/api/filters.py Outdated Show resolved Hide resolved
exotic/api/filters.py Show resolved Hide resolved
exotic/api/ld.py Outdated Show resolved Hide resolved
@tamimfatahi
Copy link
Collaborator Author

@jpl-jengelke Updated for the latest addition. EXOTIC will still need to be modified to make this work, but for now, this PR can be approved.

@tamimfatahi
Copy link
Collaborator Author

@jpl-jengelke PR should be good to go for release. Tested it on multiple different runs and outputs as expected. EXOTIC now works with the new additions.

@tamimfatahi tamimfatahi marked this pull request as ready for review August 11, 2023 11:32
@jpl-jengelke jpl-jengelke merged commit 5898ed5 into develop Aug 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Default filter to MObs CV for MObs data Grammar fix: Update GUI on new inits file: "more easy" -> "easier"
2 participants