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/PREOPS-4647: update night in prenight when visits loaded, if needed #58

Merged
merged 1 commit into from
Dec 11, 2023

Conversation

ehneilsen
Copy link
Collaborator

No description provided.

Copy link

codecov bot commented Dec 8, 2023

Codecov Report

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

Comparison is base (2eb034e) 12.24% compared to head (fdbd545) 12.53%.
Report is 4 commits behind head on main.

❗ Current head fdbd545 differs from pull request most recent head 490f425. Consider uploading reports for the commit 490f425 to get more accurate results

Files Patch % Lines
schedview/app/prenight/prenight.py 0.00% 8 Missing ⚠️
schedview/compute/astro.py 0.00% 7 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #58      +/-   ##
==========================================
+ Coverage   12.24%   12.53%   +0.29%     
==========================================
  Files          39       40       +1     
  Lines        3243     3271      +28     
  Branches      492      495       +3     
==========================================
+ Hits          397      410      +13     
- Misses       2837     2852      +15     
  Partials        9        9              

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

@ehneilsen
Copy link
Collaborator Author

I think something is wrong with codecov. It's complaining about a bunch of lines in schedview.compute.astro.compute_central_night not being tested, but I'm sure they are. Not only to I see the test case I expect to be covering them being run in the github action, I can double check that these lines are in fact covered by the test by setting a breakpoint when running locally, and walking through the lines of code when they are run with pytest.

@ehneilsen ehneilsen force-pushed the tickets/PREOPS-4647 branch from 57cc960 to fdbd545 Compare December 8, 2023 22:26
@rhiannonlynne
Copy link
Member

Yeah, the codecov report seems odd in other repos as well. I'm not entirely sure what to do about it.
The report seems not necessarily in line with what seems reasonable to me either.
I suppose a minimum thing to do would be to take the badges off the front pages (of our other repos, not added yet for schedview).

Copy link
Member

@rhiannonlynne rhiannonlynne left a comment

Choose a reason for hiding this comment

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

Does this mean the dashboard will start up on the middle night out of a set of visits? I still think the first night is probably where you'd like to start, but I suppose further testing will help understand better what's most useful.

@ehneilsen
Copy link
Collaborator Author

Yes, currently it starts in the middle. I chose that rather than the first because I've seen sims that just have one or two exposures on one night, followed by a full night on the next, in which case it's clearly the next night that is wanted.

A better approach might be use the date of the first night with more than some number of exposures, but really, when faced with a database with multiple nights, anything the code might do is a just a wild guess anyway.

I can imagine that in the future that the code that automatically generates sims for the pre-night briefing will do exactly some number of nights, and reliably not start in the middle of a night, in which case it will either not matter (if it does one), or will be easy to change to do the first night.

@ehneilsen ehneilsen merged commit 349e631 into main Dec 11, 2023
7 checks passed
@ehneilsen ehneilsen deleted the tickets/PREOPS-4647 branch December 11, 2023 17:24
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