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

Ugf newgrid for ISatSS #59

Open
wants to merge 3 commits into
base: ugf-newgrid
Choose a base branch
from

Conversation

hopesea79
Copy link

I have modified the make_grid.py and imagery.py to incorporate the ugf-newgrid into ISatSS.

@deeplycloudy
Copy link
Owner

@hopesea79 Thanks for your contribution to help get this branch ready for use in ISatSS.

I think this summarizes your changes:

  1. You propose to use the value of fill from the function argument instead of a hard-coded value. Good idea.
  2. You want to be able to override the output filename, so that it is always the same, instead of varying as a function of time. I can see how that would be useful in ISatSS, but your change breaks the naming functionality expected by the public product feed at Unidata. Outside ISatSS, the filenames need to be specific about the product in a way that matches the convention of other public GOES imagery feeds. Can your change be made into an option that doesn't change the standalone-functionality?
  3. You want to turn off scale and offset for the x and y coordinate variables, keeping them as float32 instead of as scaled int16. Is it possible to account for this in a change to ISatSS, instead?

@djhoese
Copy link
Contributor

djhoese commented Dec 20, 2019

For point 2 @deeplycloudy, what about my ideas in #54. If someone can provide a pattern as their output filename then glmtools can format it with the available metadata including datetimes. I'm not sure if the filename stuff in this PR is the same as what I was asking about in #54, but thought I'd mention it for the flexibility it provides.

@deeplycloudy
Copy link
Owner

@djhoese thanks for calling my attention to that outstanding issue. I commented over there to indicate that I think it's a good idea. I think 2. above is an additional wrinkle, where we also need to also allow an output filename template in addition to the path template.

@djhoese
Copy link
Contributor

djhoese commented Dec 20, 2019

output filename template in addition to the path template.

Or just one argument that represents the whole output path? Thanks for commenting on my issue. I'll see what I can do before leaving today.

@deeplycloudy
Copy link
Owner

I'm not picky about whether it's one argument or two … let's follow the wisdom of any available Unix conventions.

This also relates to #26 - how do we expose the GOES sector and platform ID (G16, G17) so that it can be used to generate the standard, ABI-like filenames if needed?

@dopplershift
Copy link

It's not elegant, but for my GOES restitching, I just have a function that handles making the name and there's a limited set of parameters I put into the template string:

https://github.com/Unidata/ldm-alchemy/blob/58ff661e7b3c16c83e2409012cb4b590a7b22a76/goes-restitch.py#L39-L75

@hopesea79
Copy link
Author

hopesea79 commented Dec 31, 2019 via email

@deeplycloudy deeplycloudy changed the title Ugf newgrid Ugf newgrid for ISatSS Jan 17, 2020
@deeplycloudy
Copy link
Owner

@hopesea79 I renamed this PR just now to reflect that it is a discussion about how to take the ugf-newgrid branch meant for standalone use and adapt it to ISatSS. I think anything ISatSS specific should stay on its own branch. However, some of the changes should go in ugf-newgrid directly for all users, as described below.

For 1., I would like to have these changes in ugf-newgrid, and would be happy to merge them as a standalone PR directly into ugf-newgrid.

For 2., let's revisit that once we see what @djhoese is working on re: #54. If you have input into that design, please comment on that thread. I think we should be able to accommodate @djhoese and your needs all at once. I think that change will also be merged directly to ugf-newgrid.

For 3., I think you're proposing to revert scale_and_offset to their original behavior, and not use the changes in this PR? That sounds good to me. Otherwise, if you want scale_and_offset=False you can pass it as part of the output_kwargs dict in the ISatSS call to grid_GLM_flashes. You can see how this is done on line 284 of examples/grid/make_GLM_grids.py. In either case, it would not require a change to ugf-newgrid, but rather a change to how ISatSS calls glmtools.

Let me know if this works for your needs, as I am very interested in making sure we have something that is stable and ready for ISatSS as soon as possible.

@deeplycloudy
Copy link
Owner

deeplycloudy commented Feb 15, 2020

See #63 for changes to the default output path for #54. @hopesea79 This will impact ISatSS integration, but should be a very simple change. Instead of running make_GLM_grids.py with -o /my/output/path/ you will pass -o /my/output/path/{start_time:%Y/%b/%d}/{dataset_name} to keep the same behavior. The same template string should be passed to the outdir kwarg in glmtools.grid.make_grids.grid_GLM_flashes if you are calling glmtools directly.

@hopesea79
Copy link
Author

hopesea79 commented Feb 21, 2020 via email

@deeplycloudy
Copy link
Owner

I just merged ugf-newgrid into master, and created a new isatss-ugf-newgrid branch for use in making any remaining changes that are desired for integration into ISatSS. I suggest migrating these changes over to that new branch.

@hopesea79
Copy link
Author

hopesea79 commented Mar 2, 2020 via email

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.

4 participants