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

Tickets/dm-36246 #33

Merged
merged 1 commit into from Oct 26, 2022
Merged

Tickets/dm-36246 #33

merged 1 commit into from Oct 26, 2022

Conversation

bsmartradio
Copy link
Contributor

Added simple metrics for the median and mean of the psf, aperture, and total flux. Added a plot for comparing the ratio of the psf to aperture flux.

@natelust
Copy link
Contributor

First thing I note is that the git history on this is going to need fixed up before merge. Please squash the fixups into the code. Additionally, I suspect you hit the Github button to bring your branch up to date with main. However, we rebase our branches against main instead, so you may need to drop that commit and rebase. If you need help doing any of that let me know, and we can set up a time to work through it.

@timj
Copy link
Member

timj commented Sep 28, 2022

so you may need to drop that commit and rebase

I think if you rebase, by default merge commits are dropped.

@bsmartradio
Copy link
Contributor Author

I am not sure how to squash all of my commits at this stage, but if its like Tim said and a rebase onto main would fix it I can do it that way.

@timj
Copy link
Member

timj commented Oct 6, 2022

git rebase main should remove the merge commit. You have to explicitly tell git rebase to retain merge commits (via --rebase-merges).

Comment on lines +22 to +27
"psFluxMeanMetric": "flx",
"apFluxMeanMetric": "flx",
"totFluxMeanMetric": "flx",
"psFluxMedianMetric": "flx",
"apFluxMedianMetric": "flx",
"totFluxMedianMetric": "flx",
Copy link
Contributor

Choose a reason for hiding this comment

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

Do these need to be "flx" or could you write out "flux"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As it turns out, flux is not a valid unit and would have to be added as a custom unit to work. flx, or STflux are valid units. I did test it out with flux and it broke the code.

@bsmartradio bsmartradio merged commit b56a9a1 into main Oct 26, 2022
@bsmartradio bsmartradio deleted the tickets/DM-36246 branch October 26, 2022 14:45
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

4 participants