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

Mag stats #219

Merged
merged 8 commits into from
Apr 14, 2020
Merged

Mag stats #219

merged 8 commits into from
Apr 14, 2020

Conversation

jeanconn
Copy link
Contributor

@jeanconn jeanconn commented Mar 27, 2020

Description

Add 5, 16, 50, 84, 95 mag stats to the guide stats table. This also adds some changes to support running on time ranges to make it easier to do trivial multiprocessing.

Testing

  • Passes unit tests on MacOS, linux, Windows (at least one required)
  • Functional testing

Fixes #216

# Set all columns with mag info to 99.0 initial value instead of zero
for col in table.dtype.names:
if 'aoacmag' in col:
table[col] = 99.0
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is somewhat reasonable to "initialize" the numpy table with values of 99 for all the aoacmag_* cols if we want 99.0 for that kind of missing data. Not sure if other columns should get that treatment too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is probably the only controversial thing in here. As with everything, hard to know how much to rummage around and improve things and how much to just back away slowly.

@jeanconn jeanconn changed the title WIP: Mag stats Mag stats Apr 6, 2020
@@ -112,7 +115,7 @@
('var', 'int'),
('pos_err', 'int'),
('mag_aca', 'float'),
('mag_err', 'int'),
('mag_aca_err', 'int'),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @taldcroft and @javierggt ! It sounds like you also want a change in this PR somewhere to scale the mag_aca_err column to units of mag. That's fine. Though should that be in mica or in agasc.get_star (or get_stars) with a calibrate flag or some such?

Copy link
Member

Choose a reason for hiding this comment

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

Well given that all the rest of this is uncalibrated it appears we cannot make a change here without having a one-off inconsistency. Just leave it. And don't bother with making an agasc issue. If one of us gets annoyed enough one day we'll do it, but it is a low priority and we don't need more low priority issues.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would it be less inconsistent to just UPPER CASE these?

Copy link
Member

Choose a reason for hiding this comment

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

It would be more consistent to upper case them, but would break all existing code that uses these. Not worth the trouble I'd say.

@jeanconn jeanconn requested a review from taldcroft April 8, 2020 20:06
Copy link
Member

@taldcroft taldcroft left a comment

Choose a reason for hiding this comment

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

LGTM!

@jeanconn jeanconn merged commit 8fad77f into master Apr 14, 2020
@jeanconn jeanconn deleted the mag_stats branch April 14, 2020 22:53
@javierggt javierggt mentioned this pull request Apr 21, 2020
17 tasks
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.

Add guide star 5th, 16th, 50th, 84th, and 95th percentile mag columns to guide stats.
3 participants