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

Script to export clock files #1373

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

dlakaplan
Copy link
Contributor

I think this is what was requested in #1372 . But I don't know if a better implementation is available.

Usage:

export_clocks -o ./ gbt arecibo chime vla --log-level=DEBUG

So this runs as an executable script. It could also be made into a standalone function if needed, although what is needed for that is very simple.

@dlakaplan dlakaplan requested a review from aarchiba August 8, 2022 18:46
@dlakaplan
Copy link
Contributor Author

It could also have a pre-determined set of observatories

@codecov
Copy link

codecov bot commented Aug 8, 2022

Codecov Report

Merging #1373 (e6c8a60) into master (4ce58a8) will decrease coverage by 0.06%.
The diff coverage is 11.53%.

❗ Current head e6c8a60 differs from pull request most recent head 986ff18. Consider uploading reports for the commit 986ff18 to get more accurate results

@@            Coverage Diff             @@
##           master    #1373      +/-   ##
==========================================
- Coverage   62.19%   62.12%   -0.07%     
==========================================
  Files          89       90       +1     
  Lines       20215    20241      +26     
  Branches     3650     3652       +2     
==========================================
+ Hits        12572    12575       +3     
- Misses       6857     6880      +23     
  Partials      786      786              
Impacted Files Coverage Δ
src/pint/scripts/export_clocks.py 0.00% <0.00%> (ø)
src/pint/observatory/topo_obs.py 80.57% <100.00%> (+0.42%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

dest="directory",
help="Destination directory (set $PINT_CLOCK_OVERRIDE to this value to look here first for clock files)",
)
parser.add_argument(
Copy link
Contributor

Choose a reason for hiding this comment

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

More conventional would be a --verbose (possibly repeatable) and optionally a --quiet rather than explicitly referencing log levels. Since this is a specific application, one might be able to select sensible log levels to correspond to these.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's the type of interface I had gone for initially for applications, but @scottransom wanted this method

Copy link
Member

Choose a reason for hiding this comment

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

I'm a bit confused... What does --verbose have to do with a specified output directory or filename?

Copy link
Member

Choose a reason for hiding this comment

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

And if the discussion is about the log levels, I was just thinking that we should be consistent with the other PINT command line programs. Maybe --verbose could correspond to upping the log level by one, as well. (maybe by one for each verbose added).

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, this is about log levels. I'm not sure users of a command line program will have any idea what log levels have to do with what output they want to see. Making --verbose bump up the log level, and --quiet bump it down by one might be a tidy way to present a more conventional command-line interface.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is what I had implemented months ago. I agree it's more standard but I don't really care either way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See #1377 for discussion of this.

@aarchiba
Copy link
Contributor

It's worth pointing out that I don't think this is relevant to #1372 - that is a request to be able to preload the astropy cache for use by machines without internet access (or containers that should minimize calls to the internet). It might have some use for non-PINT users who just want to pull out the clock files without having to face a line of PINT code?

@dlakaplan
Copy link
Contributor Author

OK, if this doesn't do what #1372 wants (I thought it could, since it would then not have calls to check all of the clock files over the internet, but maybe I'm misunderstanding) then it's fine to close

@aarchiba
Copy link
Contributor

OK, if this doesn't do what #1372 wants (I thought it could, since it would then not have calls to check all of the clock files over the internet, but maybe I'm misunderstanding) then it's fine to close

I think @JPGlaser would have to confirm for sure, but we've discussed it before and the problem is how to pre-populate the Astropy cache for use on cluster nodes that will not have Internet access. I've added many more comments to the issue.

@paulray
Copy link
Member

paulray commented Aug 16, 2022

Should we close this, or merge it?

@dlakaplan
Copy link
Contributor Author

Whatever @aarchiba thinks. It may not be useful, but I don't think it's harmful. However, merging with #1380 would be needed

@aarchiba
Copy link
Contributor

I don't know how useful this is - I'm not sure I have a use case in mind for it. It's not much code, though, so if it had tests to ensure we didn't break it then it wouldn't do any harm to include it.

@abhisrkckl
Copy link
Contributor

What is the status of this?

@dlakaplan
Copy link
Contributor Author

Still tabled. I think it works for what it is, but it's not clear that it's what's needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants