Skip to content

Conversation

@TejasMorbagal
Copy link
Member

@TejasMorbagal TejasMorbagal commented Jan 6, 2025

Implemented CLI and python API that enables generating OSC collection from a DeepESDL dataset and publishing it to open science catalog as a static file in open-science-metadata repository.

@TejasMorbagal TejasMorbagal requested a review from forman January 6, 2025 14:56
@TejasMorbagal TejasMorbagal self-assigned this Jan 6, 2025
@TejasMorbagal TejasMorbagal added the enhancement New feature or request label Jan 6, 2025
@TejasMorbagal TejasMorbagal requested review from forman and pont-us and removed request for forman and pont-us January 6, 2025 15:07
Copy link
Contributor

@forman forman left a comment

Choose a reason for hiding this comment

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

Looks good overall!

  • See all my suggestion and comments
  • Copyright (c) 2024 --> 2025
  • In CLI, configure logger to print nicely to stdout

README.md Outdated
execute the steps below:

```commandline
conda env create -f environment.yml
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
conda env create -f environment.yml
conda env create

f"Last error: {last_exception}"
)

def _get_spatial_extent(self) -> SpatialExtent:
Copy link
Contributor

Choose a reason for hiding this comment

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

Refactor function body: code triplication

# For irregular gridding
x_min, x_max = (float(self.dataset.x.min()), float(self.dataset.x.max()))
y_min, y_max = (float(self.dataset.y.min()), float(self.dataset.y.max()))
return SpatialExtent([[x_min, y_min, x_max, y_max]])
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this ok? Transform to geographic using grid mapping?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it works for irregular gridding with y,x coordinates. Tested it with polar dataset to confirm. For a pystac collection, I just need the bbox that encompasses the data. Any other considerations here ?



@click.command(name="publish-dataset")
@click.option(
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer, you would not force users putting credentials into a text file.
If it is really required for some reason, make it a hidden file with known name that is picked up from the root of the repo and without extension. So:

  • Remove CLI option.
  • Name the file .gitaccess. A template could be created by deep-code new ....
  • Provide env-var alternative: GH_TOKEN, GH_USER. Let the tool document this.

@TejasMorbagal TejasMorbagal merged commit c363d08 into main Jan 10, 2025
1 check passed
@TejasMorbagal TejasMorbagal deleted the tejas-xxx-implement-publish-product-feature branch January 10, 2025 09:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants