Skip to content

JPL Notebook#162

Open
smk0033 wants to merge 7 commits intostagingfrom
jpl-review
Open

JPL Notebook#162
smk0033 wants to merge 7 commits intostagingfrom
jpl-review

Conversation

@smk0033
Copy link
Contributor

@smk0033 smk0033 commented Apr 24, 2025

Adding the JPL notebook and supporting submodule file for Science Support Team to review. Github ticket

@smk0033 smk0033 added the documentation Improvements or additions to documentation label Apr 24, 2025
@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@smk0033 smk0033 requested a review from wildintellect April 24, 2025 14:42
@github-actions
Copy link

github-actions bot commented Apr 24, 2025

PR Preview Action v1.6.3

🚀 View preview at
https://US-GHG-Center.github.io/ghgc-docs/pr-preview/pr-162/

Built to branch gh-pages at 2025-12-16 18:50 UTC.
Preview will be ready when the GitHub Pages deployment is complete.

@wildintellect
Copy link

wildintellect commented Apr 24, 2025

@Jeanne-le-Roux I have concerns with the Copyright selected. Is there a reason they can't use something like CC--BY-NC

As is the licensing makes it unclear that anyone can use the code or modify it.

I'll also note the Quarto cite is CC-BY, and Code repo is currently Apache 2.0

@review-notebook-app
Copy link

review-notebook-app bot commented Apr 24, 2025

View / edit / reply to this conversation on ReviewNB

wildintellect commented on 2025-04-24T17:31:54Z
----------------------------------------------------------------

No mention of which compute environment or minimum/suggested size is needed to execute this on the hub. Guessing we want Pangeo, but unclear on Ram selection.


@review-notebook-app
Copy link

review-notebook-app bot commented Apr 24, 2025

View / edit / reply to this conversation on ReviewNB

wildintellect commented on 2025-04-24T17:31:55Z
----------------------------------------------------------------

Can we suggest a reference at least which field of study this is. I'm guessing this is Inverse Modeling in statistics https://www.sciencedirect.com/science/article/pii/S0168169924000152 based on mention in https://dus.jpl.nasa.gov/projects/flux-inversion/

More background https://www.sciencedirect.com/topics/earth-and-planetary-sciences/atmospheric-inversion


@review-notebook-app
Copy link

review-notebook-app bot commented Apr 24, 2025

View / edit / reply to this conversation on ReviewNB

wildintellect commented on 2025-04-24T17:31:55Z
----------------------------------------------------------------

Line #9.    from inventory_evaluation_helper import *  # Import all functions from the inventory evaluation helper module

This should be the last import as it's local.

Reminds me that we should enable nbqa https://github.com/nbQA-dev/nbQA like VEDA-docs does to fix general python linting issues.

Linting will also catch the use of Wildcard imports * which are always discouraged due to security concerns.


@review-notebook-app
Copy link

review-notebook-app bot commented Apr 24, 2025

View / edit / reply to this conversation on ReviewNB

wildintellect commented on 2025-04-24T17:31:56Z
----------------------------------------------------------------

Eliminate the cause of this error "Country United States not found in the shapefile."


@review-notebook-app
Copy link

review-notebook-app bot commented Apr 24, 2025

View / edit / reply to this conversation on ReviewNB

wildintellect commented on 2025-04-24T17:31:57Z
----------------------------------------------------------------

Consider applying a Projection more appropriate to the display of US data. Is Equal Area or Shape Distortion more important?


@wildintellect
Copy link

I reviewed the general text and code, comments above.

To be reviewed:

  • Exectuting the code
  • The scientific text for legibility

@smk0033
Copy link
Contributor Author

smk0033 commented Oct 27, 2025

Bringing @sudshu in as well for collaboration and to track progress. I added a cell to bring in the data files from Zenodo (I think it can be optimized which other members of Science Support can weigh in on, but I was able to run the entire notebook with it), and also added engine = "netcdf4" to the helper files for xarray. In addition, I formatted a raw cell at the top to match formatting with our other notebooks, and added packages for the Zenodo cell.

@smk0033 smk0033 self-assigned this Oct 27, 2025
@HarshiniGirish
Copy link

Only notebooks can be reviewed on ReviewNB so just pasting all the comments for
user_data_notebooks/user_notebooks/inventory_evaluation_helper.py here

  1. Using from pylab import * at the top of the module clutters the namespace and makes it unclear which functions come from NumPy/matplotlib.
    Replace it with explicit imports like import numpy as np and import matplotlib.pyplot as plt, and use np.zeros, np.arange, etc., to keep the module cleaner and safer to import.

2.In get_country_geometry, the "not found" message is inside the loop, so it prints once for every non-matching country, creating noisy and misleading output.
Only handle the match inside the loop, then after the loop either print a single "not found" message or raise a ValueError if no match was found.

3.In give GOSATdata, emiss_values is computed inside the loop but never used, which is confusing (please check again before removing it)

  1. In makeGeoPandasDataFrame / makeGeoPandasDataFrameFOG, USA_area_fraction is now calculated as (intersection with USA / total cell area), but the comment still describes it as “1 minus the fraction in Canada and Mexico,” which is no longer true.
    Update the comment to reflect the current calculation (e.g., “fraction of each grid cell inside the U.S. boundary”) or re-implement the Canada/Mexico subtraction explicitly and keep the comment consistent.

apart form the code looks really good @smk0033 . Thank you

@review-notebook-app
Copy link

review-notebook-app bot commented Nov 18, 2025

View / edit / reply to this conversation on ReviewNB

HarshiniGirish commented on 2025-11-18T22:02:58Z
----------------------------------------------------------------

Add a simple check at the start of the cell to skip the download when data_folder_path is already populated, and include a short comment explaining that this cell downloads the Zenodo benchmark data and only needs to be run once. This avoids repeated downloads and makes rerunning the notebook much smoother.


@HarshiniGirish
Copy link

@smk0033 I've run the notebook in the ghg hub too, apart from the above the code looks good to me. Thank you

@smk0033
Copy link
Contributor Author

smk0033 commented Dec 9, 2025

Only notebooks can be reviewed on ReviewNB so just pasting all the comments for user_data_notebooks/user_notebooks/inventory_evaluation_helper.py here

  1. Using from pylab import * at the top of the module clutters the namespace and makes it unclear which functions come from NumPy/matplotlib.
    Replace it with explicit imports like import numpy as np and import matplotlib.pyplot as plt, and use np.zeros, np.arange, etc., to keep the module cleaner and safer to import.

2.In get_country_geometry, the "not found" message is inside the loop, so it prints once for every non-matching country, creating noisy and misleading output. Only handle the match inside the loop, then after the loop either print a single "not found" message or raise a ValueError if no match was found.

3.In give GOSATdata, emiss_values is computed inside the loop but never used, which is confusing (please check again before removing it)

  1. In makeGeoPandasDataFrame / makeGeoPandasDataFrameFOG, USA_area_fraction is now calculated as (intersection with USA / total cell area), but the comment still describes it as “1 minus the fraction in Canada and Mexico,” which is no longer true.
    Update the comment to reflect the current calculation (e.g., “fraction of each grid cell inside the U.S. boundary”) or re-implement the Canada/Mexico subtraction explicitly and keep the comment consistent.

apart form the code looks really good @smk0033 . Thank you

Hi @sudshu! I've made a few updates to pull in the Zenodo data. Our Science Support team aided in reviewing, and I've addressed comment 2 here. Would you like to weigh in on comments 1, 3, and 4 and have me address those as well? You are also more than welcome to push updates to this branch if you wish at any time! I think this is about ready to send to Emily for her review.

@sudshu
Copy link

sudshu commented Dec 15, 2025

@smk0033 your suggestions look good to me. Feel free to implement those.

Copy link

@HarshiniGirish HarshiniGirish left a comment

Choose a reason for hiding this comment

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

Thanks @smk0033. I ended up overlooking this bit

  1. EPA scrollable maps issue : Under # EPA inventory, the code cell that starts “Create a GeoPandas DataFrame … for the "epa" inventory” (gdf_epa = ...). It does not use with out: and the for-loop indentation is off, so the out widget ends up empty and nothing is actually scrollable.

  2. add imports Output

Copy link

@HarshiniGirish HarshiniGirish left a comment

Choose a reason for hiding this comment

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

Thanks @smk0033. I ended up overlooking this bit

  1. EPA scrollable maps issue : Under # EPA inventory, the code cell that starts “Create a GeoPandas DataFrame … for the "epa" inventory” (gdf_epa = ...). It does not use with out: and the for-loop indentation is off, so the out widget ends up empty and nothing is actually scrollable.

  2. add imports Output

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

Labels

documentation Improvements or additions to documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants