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.

Sign up for GitHub

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

Jump to bottom

Add checks for observability and grid plot #22

Open
wants to merge 21 commits into
base: main
Choose a base branch
from
Open

Add checks for observability and grid plot #22

wants to merge 21 commits into from

Conversation

HealthyPear
Copy link
Member

@HealthyPear HealthyPear commented Sep 19, 2024 •
edited
Loading

Closes #16 .

Also adds preliminary checks if the source is ever observable and which months are best to observe it.

Also adds plots like these

observability_heatmap_Crab Nebula

this one only if selected time range is not more than 1 day

observability_grid_Crab Nebula

@HealthyPear HealthyPear added enhancement New feature or request observability labels Sep 19, 2024
@HealthyPear HealthyPear force-pushed the plots-observation_grid branch from f079573 to b1a066f Compare September 20, 2024 10:23
@review-notebook-app Review Notebook App
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

@HealthyPear HealthyPear added this to the v0.1.0 milestone Sep 23, 2024
@HealthyPear HealthyPear force-pushed the plots-observation_grid branch 3 times, most recently from 6b2bcbc to 5caccd8 Compare October 31, 2024 18:17
@HealthyPear HealthyPear marked this pull request as ready for review October 31, 2024 20:12
aleberti
aleberti suggested changes Nov 14, 2024
View reviewed changes
src/iact_estimator/observability.py

def check_observability(
constraints, observer, target_source, time_range, time_grid_resolution
):
Copy link

Choose a reason for hiding this comment

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

Missing docs for this function.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

src/iact_estimator/plots.py
ax=None,
savefig=True,
output_path=None,
):
Copy link

Choose a reason for hiding this comment

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

Missing docs.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

src/iact_estimator/scripts/main.py
Comment on lines +163 to +164
logger.info("Observation starts at %s", start_datetime)
logger.info("Observation ends at %s", end_datetime)
Copy link

Choose a reason for hiding this comment

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

I noticed that sometimes you use f-strings and here the simple format. I would use f-strings everywhere if possible, you can force a certain format also for f-strings if needed (I guess here you need it to print the Time object as string representation, but I would assume it should work also with f-strings).

Copy link
Member Author

Choose a reason for hiding this comment

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

The reason is this
https://realpython.com/python-f-strings/#lazy-evaluation-in-logging

@aleberti
Copy link

I tried the example notebook for a few different settings, to check that e.g. for the Crab I get 0 hours if I put a very limited zenith range below its culmination zenith.

As additional comments:

  • can you add in the user guide doc page how to call the help? I know it is in the API reference, but you could add a sentence like "For further options, please run iact-estimator run -h or iact-estimator config -h"
  • I tried the script and it would be nice to have in the output a log line saying the path+filename of the output file. NOw it just says that it finished but no information about where the plots were put (I mean specifically the filenames, the path is either the cwd or the one set via the command line argument).

@HealthyPear
Add new observability module
dca5141 
Fix bug in observability module

fix formatting issue when definining AtNightConstraint
@HealthyPear
Add plot_observability_constraints_grid
5c41075
@HealthyPear
Update main with observability check
38603f5 
Update call to check_observability

Fix call to observation datetime from main.py

Update call to source_detection in main.py
@HealthyPear
Update config file with new "observation" section
477dc84
@HealthyPear
Add news fragment for PR#22
b07d5ca
@HealthyPear
Update docs notebooks for new call to observation time
5e446a9
@HealthyPear
core.py : Fix call to observation time
74a22ed
@HealthyPear
observability.py : use max solar altitude
f685adb
@HealthyPear
main.py : update
7338388
@HealthyPear
plots.py : update for observability grid
bfe5eaa
@HealthyPear
config.yml : update time grid options
4b2c24e
@HealthyPear
Add observability.get_days_in_this_year
24cd993 
to account for leap years in functions which depend on year length
@HealthyPear
Update plots module
948706d 
- add observability heatmap
- fixed docstrings
- improved label appearance in observability grid
@HealthyPear
Add illumination fraction constraint and no max separation
f2152c1
@HealthyPear
Update main script
017966e 
- call observability heatmap
- call observability grid only if within a day
@HealthyPear
Update example notebook for observability heatmap
cf514d9
@HealthyPear
Fix plot_observability_constraints_grid time resolution and x labels …
754b6cf 
…size
@HealthyPear HealthyPear force-pushed the plots-observation_grid branch from 5caccd8 to 754b6cf Compare November 14, 2024 14:18
@HealthyPear
Copy link
Member Author

can you add in the user guide doc page how to call the help?

done

output a log line saying the path+filename of the output file

done

Thanks! Let me know if there is something else missing or not working!

@HealthyPear HealthyPear requested a review from aleberti December 5, 2024 14:01
@HealthyPear
Copy link
Member Author

This PR also closes #9 unless something else comes up before final approval.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SeiyaNozaki SeiyaNozaki Awaiting requested review from SeiyaNozaki

@aleberti aleberti Awaiting requested review from aleberti

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
enhancement New feature or request observability
Projects
None yet
Milestone
v0.1.0
Development

Successfully merging this pull request may close these issues.

Add plot for observational contraints along a time range
2 participants
@HealthyPear @aleberti