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

Improve testing of pocs.scheduler.field.Field #574

Merged
merged 1 commit into from
Sep 3, 2018

Conversation

jamessynge
Copy link
Contributor

Require that the field name be non-empty.

This is a precursor to adding support for colon separated hour angle or degree values.

Require that the field name be non-empty.
@jamessynge jamessynge requested a review from wtgee August 31, 2018 17:40
@codecov
Copy link

codecov bot commented Aug 31, 2018

Codecov Report

Merging #574 into develop will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #574      +/-   ##
===========================================
- Coverage    70.52%   70.51%   -0.01%     
===========================================
  Files           64       64              
  Lines         5520     5522       +2     
  Branches       768      769       +1     
===========================================
+ Hits          3893     3894       +1     
  Misses        1420     1420              
- Partials       207      208       +1
Impacted Files Coverage Δ
pocs/scheduler/field.py 100% <100%> (ø) ⬆️
pocs/dome/astrohaven.py 71.05% <0%> (-1.76%) ⬇️
pocs/dome/protocol_astrohaven_simulator.py 79.74% <0%> (-0.85%) ⬇️
pocs/observatory.py 85.45% <0%> (-0.3%) ⬇️
pocs/serial_handlers/protocol_arduinosimulator.py 76.92% <0%> (+1.53%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d1f411e...66b93a6. Read the comment docs.

@jamessynge
Copy link
Contributor Author

As noted elsewhere, we have an issue with our coverage info. The only file that should see any change is pocs/scheduler/field.py, but we see changes to 4 other files. Perplexing.

@jamessynge
Copy link
Contributor Author

And interestingly, those 4 changes are in areas I've worked on: dome and arduino. Not sure if that is coincidence, or if my test style results in inconsistent coverage.

Copy link
Member

@wtgee wtgee left a comment

Choose a reason for hiding this comment

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

This looks fine, but since the position field is just passed directly to SkyCoord this is not really exercising our code but instead exercising theirs. I understand that this is in anticipation of different input but perhaps it just makes more sense to change position to a valid SkyCoord instance.

If not, the change that is going to be required is that you are going to have to pass the units to Field otherwise it can't disambiguate the : syntax. But that will just make the targets file more complex, not less. The convenience of having : will be somewhat lost since you'll just have to specify (or assume) the units anyway.

@jamessynge
Copy link
Contributor Author

Not surprisingly, I've already tested the solution that I have in mind, which doesn't require that units be passed in to field. Instead, I modify Field to pass the units (hourangle and degree) into SkyCoord, which handles both the existing format and the new one. Does that seem OK?

@wtgee
Copy link
Member

wtgee commented Sep 2, 2018

Not surprisingly, I've already tested the solution that I have in mind, which doesn't require that units be passed in to field. Instead, I modify Field to pass the units (hourangle and degree) into SkyCoord, which handles both the existing format and the new one. Does that seem OK?

Actually, no, that limits our flexibility. Besides just entering it with a colon delimiter someone might want to just enter the actual degrees. Or, more likely, as we go toward charting TESS fields we will probably be using galactic coordinates rather than RA/Dec. I think it makes more sense to pass an actual SkyCoord to the Field.

@jamessynge
Copy link
Contributor Author

What syntax would you propose for the Galactic Coords? Or more generally, what should we put in the targets file to support creating a SkyCoord to be passed into a Field ctor?

@wtgee
Copy link
Member

wtgee commented Sep 2, 2018

The existing syntax is what I would propose since the unit is contained within the string.

I don't think switching the : to the appropriate unit is a heavy burden on someone and in the future the hope is people won't be interacting with the yaml files directly anyway so it would be a non-issue. But there is no reason to limit what a Field can accept for SkyCoord params when really it should work with any SkyCoord. I honestly don't see the need to support a unit-less string.

@jamessynge
Copy link
Contributor Author

I meant "what syntax will be used with galactic coords?", and "how is that differentiated from the existing RA and DEC?".

@wtgee
Copy link
Member

wtgee commented Sep 3, 2018

Galactic coordinates make most sense simply in degrees of longitude (l) and latitude (b). We will also have to change the coordinate frame in that case, which means the Field needs to either know about the position, the units, and the frame, or it just needs to accept a SkyCoord.

I'm not opposed to having Field do some parsing and properly build a SkyCoord and ideally it should probably accept either. Might just need to have a larger conversation about the Fields and scheduler related changes?

What is the current goal for this PR and the related link above? To make it easier to add things to the targets file (and make more robust)? Or is this also work toward a larger change with the Fields?

(And I think this PR is fine regardless so maybe we should merge and move this conversation?)

@jamessynge
Copy link
Contributor Author

OK, will merge and take up the discussion elsewhere.

@jamessynge jamessynge merged commit cd792ef into panoptes:develop Sep 3, 2018
@jamessynge jamessynge deleted the test-field branch September 4, 2018 00:19
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