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

This rearranges some of the photometry settings and adds an AperturePhotometry class #275

Merged
merged 15 commits into from
Feb 22, 2024

Conversation

mwcraig
Copy link
Contributor

@mwcraig mwcraig commented Feb 16, 2024

There are still a couple things to be resolved:

  • Should single/multi image photometry become methods of AperturePhotometry?
  • Is AperturePhotometry even needed?

Marking this as a draft for the moment, but if you get a chance to take a peek, @JuanCab, feedback would be welcome. WOn't get back to it myself until Sunday.

@mwcraig mwcraig requested a review from JuanCab February 16, 2024 22:08
@mwcraig mwcraig marked this pull request as draft February 16, 2024 22:08
@codecov-commenter
Copy link

codecov-commenter commented Feb 16, 2024

Codecov Report

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

Comparison is base (f63f2cc) 70.22% compared to head (3bd9301) 70.53%.
Report is 3 commits behind head on main.

❗ Current head 3bd9301 differs from pull request most recent head fece598. Consider uploading reports for the commit fece598 to get more accurate results

Files Patch % Lines
stellarphot/photometry/photometry.py 96.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #275      +/-   ##
==========================================
+ Coverage   70.22%   70.53%   +0.31%     
==========================================
  Files          24       24              
  Lines        2992     3024      +32     
==========================================
+ Hits         2101     2133      +32     
  Misses        891      891              

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

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.

So far looks like a strong first step re-organizing the settings to be more clearly delineated and also to work toward the object-oriented model of photometry we talked about. Not "approving" because it is a draft, but looks like a lot of good stuff here.

docs/index.rst Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Obviously this will need some more fleshing out, but I like that you are getting the documentation in place. You may want to mention for situations where the users may not be comfortable yet with the command line or Python programming, a Jupyter-notebook-based set of tools is available to configure stellarphot and run the photometry.

Also, do we want to note this is meant to be used on REDUCED images (or am I wrong here)? I don't remember any processing of flats/darks/etc, which I believe is because we were working with reduced images.

file_or_directory : str or Path
The file or directory on which to perform aperture photometry.

fname : str, optional (Default: None)
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't fname redundant if the value of file_or_directory already automatically switches whether it is single image processing or multi-image processing based on it being a file or directory. Do you want to support wildcard (e.g. glob) processing of filename?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point about the file name!

I think for the moment maybe we stick with just a directory for multi-image but move towards being able to process anything the ccdproc ImageFileCollection can handle.

stellarphot/settings/models.py Show resolved Hide resolved
stellarphot/settings/models.py Show resolved Hide resolved
stellarphot/settings/models.py Show resolved Hide resolved
@mwcraig mwcraig requested a review from JuanCab February 19, 2024 19:30
@mwcraig mwcraig marked this pull request as ready for review February 19, 2024 19:30
@mwcraig
Copy link
Contributor Author

mwcraig commented Feb 19, 2024

Next up will be better documentation and fleshing out the couple of widgets that still need to be done.

@mwcraig
Copy link
Contributor Author

mwcraig commented Feb 19, 2024

@JuanCab what do you think about these two questions:

  • Should single/multi image photometry become methods of AperturePhotometry?
  • Is AperturePhotometry even needed?

@JuanCab
Copy link
Contributor

JuanCab commented Feb 22, 2024

@JuanCab what do you think about these two questions:

  • Should single/multi image photometry become methods of AperturePhotometry?
  • Is AperturePhotometry even needed?

Unless you somehow want to leave the possibility open for other kinds of photometry to be done as single or multi-image, it seems to make sense to me to keep AperturePhotometry as descriptive of the task it handles. I could see that just to avoid being overly long with the function names which might call them single_image and multi_image since the 'photometry' would be redundant if they are in the AperturePhotometry class.

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 had to use Sublime Merge to see the changes since GitHub wasn't as clear about what changed since the last update. But the changes all look reasonable.

@mwcraig mwcraig merged commit 0c3170a into feder-observatory:main Feb 22, 2024
9 checks passed
@mwcraig mwcraig deleted the photometry-class branch April 29, 2024 15:39
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.

None yet

3 participants