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

Update supplement MAG_ACA_ERR test #52

Merged
merged 2 commits into from
Nov 13, 2020
Merged

Conversation

jeanconn
Copy link
Contributor

Description

Some of the updated values from the supplement matched the catalog
values in the test on the integer MAG_ACA_ERR. This change updates the test to confirm that the errors are smaller
or the same coming from the supplement (which may not generalize but appears true here).

I'm still not sure if we have different supplement files in the wild.

Testing

  • Passes unit tests on linux
  • Functional testing

Fixes #

Some of the updated values from the supplement matched the catalog
values in this check on the integer MAG_ACA_ERR.
@jeanconn jeanconn requested a review from taldcroft November 13, 2020 00:23
@javierggt
Copy link
Contributor

I can confirm that with this change the test passes with the new supplement. I'm not sure why test the inequality though. Anyway, better than nothing.

@javierggt
Copy link
Contributor

and I can confirm these are the failing tests. It passes when they are skipped.

@jeanconn
Copy link
Contributor Author

Right, I was largely just brainstorming here. It seems like we are not guaranteed anything about the new errors but they did turn out to be smaller.

@jeanconn
Copy link
Contributor Author

Tom comments: not OK because it allows for all of them to be the same. So it could be rewritten as np.any(stars2[.] != stars1[..]). I.e. at least some of them are different.

I'll update with that idea.

@jeanconn
Copy link
Contributor Author

And I just realized I don't know what you meant by stars2[.] and stars1[..] so I just checked the [ok] set.

@jeanconn jeanconn merged commit bb41340 into master Nov 13, 2020
@jeanconn jeanconn deleted the supplement_test_tweak branch November 13, 2020 01:03
@javierggt javierggt mentioned this pull request Dec 7, 2020
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.

2 participants