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

Add pydantic/ipyautoui aperture settings #154

Merged
merged 18 commits into from
Aug 18, 2023

Conversation

mwcraig
Copy link
Contributor

@mwcraig mwcraig commented Aug 9, 2023

This makes the aperture settings a pydantic model in prep for making all settings saveable.

To do before this is ready for review:

  • Redraw seeing plot if aperture_settings value changes
  • Save properly
  • Read settings in notebook 03
  • Rewrite the multi-image photometry signature to use ApertureSettings
  • Rewrite single-image photometry signature to use ApertureSettings

@mwcraig mwcraig added the refactor Summer 2023 project to rewrite stellarphot label Aug 9, 2023
@mwcraig mwcraig marked this pull request as draft August 9, 2023 18:25
@mwcraig mwcraig force-pushed the add-aperture-settings branch from 35a36c0 to 105041b Compare August 10, 2023 17:03
@mwcraig mwcraig force-pushed the add-aperture-settings branch from de64722 to 086c579 Compare August 10, 2023 18:37
@mwcraig mwcraig marked this pull request as ready for review August 16, 2023 01:37
@mwcraig mwcraig requested a review from JuanCab August 16, 2023 01:37
@mwcraig
Copy link
Contributor Author

mwcraig commented Aug 16, 2023

@JuanCab -- I think this is ready for a look. Not sure about the whole approach of making a separate settings module, but at least it would keep things organized.

Copy link
Contributor

@JuanCab JuanCab left a comment

Choose a reason for hiding this comment

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

Looks good as a means of getting the aperture settings read in and passed to the photometry. There are some places where I noted possible tweaks to the documentation, but otherwise it's mostly me just asking questions of why you made a choice or confirming my understanding of the function/class behavior.

setup.cfg Show resolved Hide resolved

from enum import Enum

from astropy import units as un
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a complaint, just curious why you switched to units as un instead of units as u, which seems more conventional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

😬 the typing expansion program changes u to you so I used un. I can change that....

Copy link
Contributor

Choose a reason for hiding this comment

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

Not to be difficult, but can we be consistent and use u for astropy.units?

stellarphot/settings/models.py Show resolved Hide resolved
stellarphot/photometry/photometry.py Show resolved Hide resolved
stellarphot/plotting/aij_plots.py Outdated Show resolved Hide resolved
stellarphot/plotting/aij_plots.py Show resolved Hide resolved
stellarphot/plotting/aij_plots.py Outdated Show resolved Hide resolved
Comment on lines +1 to +2
from .models import *
from .views import *
Copy link
Contributor

Choose a reason for hiding this comment

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

Just so you know, ruff really hates * on imports (and PEP8 isn't terribly pleased with it either). I am wondering if despite the pain we should be explicit with the function imports. I know __all__ is controlling this to some extent, but I wonder if linters are happier in part because they know where to look for class/function definitions in these cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK; I had thought this was the one case it was OK to get the namespace the way you want it.

@mwcraig mwcraig changed the title WIP: Add pydantic/ipyautoui aperture settings Add pydantic/ipyautoui aperture settings Aug 17, 2023
Copy link
Contributor

@JuanCab JuanCab left a comment

Choose a reason for hiding this comment

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

I think if you can fix astropy.units in models.py to be addressed as u (for consistency with the rest of the code we use), this can be merged.


from enum import Enum

from astropy import units as un
Copy link
Contributor

Choose a reason for hiding this comment

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

Not to be difficult, but can we be consistent and use u for astropy.units?

@mwcraig
Copy link
Contributor Author

mwcraig commented Aug 17, 2023

I think if you can fix astropy.units in models.py to be addressed as u (for consistency with the rest of the code we use), this can be merged.

Done for real this time -- actually, I removed the import because it and the aperture units thing were not the way to go. I'll stick to u in the future though

@mwcraig mwcraig force-pushed the add-aperture-settings branch from 3c2ff78 to 56ece82 Compare August 17, 2023 18:11
@JuanCab JuanCab merged commit 4407138 into feder-observatory:main Aug 18, 2023
@mwcraig mwcraig deleted the add-aperture-settings branch February 8, 2024 15:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Summer 2023 project to rewrite stellarphot
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants