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

[REVIEW] Update language/styling based on reviewer's feedback #8

Open
20 of 21 tasks
bnubald opened this issue Jun 3, 2024 · 0 comments
Open
20 of 21 tasks

[REVIEW] Update language/styling based on reviewer's feedback #8

bnubald opened this issue Jun 3, 2024 · 0 comments
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@bnubald
Copy link
Owner

bnubald commented Jun 3, 2024

Reviewer 1

Update based on reviewer comments by weiji14 here:

eds-book-gallery#5

Fixed inconsistency, and added link to U-Net model's paper on first mention.

where sea ice does not form, land regions, and the polar hole.

Some parameters are fed to predict_forecast that ideally shouldn't need to be specified (like seed and n_filters_factor) and might seem contextually odd. They're used to locate the appropriate saved network. This will be cleaned up in a future version.

  • Is the cleaned up version meant to be completed by this review? Or in a future as in after June 2024?
    Also, what does n_filters_factor do? Not immediately obvious. Edit: Seems to be documented in the 'Improving results' subsection below, maybe bring that up here.

This would be cleaned up in a future version after June 2024. Also, for n_filters_factor, added a brief description in parenthesis on first mention.

  • State "NumPy (.npy) files", user might not know that numpy files = .npy files.

Added this change.

Line #17.        fc = ds.sic_mean.isel(time=0).drop_vars("time").rename(dict(leadtime="time"))
Line #18.        fc['time'] = [pd.to_datetime(forecast_date) \
                + dt.timedelta(days=int(e)) for e in fc.time.values]
  • These lines might benefit from some inline code comments to explain what is happening for users not adept at xarray/pandas.

Added inline explanation for these lines of code.

Line #21.        anim = xvid(fc, 15, figsize=4, mask=land_mask)
  • Is there a way to label the colorbar with something like "Sea-ice concentration fraction"? Maybe a feature request to the IceNet library?

Created an issue on IceNet repo for this icenet-ai/icenet#269, aiming for v0.2.9 release

Reviewer 2

Update based on William-gregory's comments from here:

alan-turing-institute/environmental-ds-book#239 (comment)

  • No ORCID, but names, affiliations and GitHub handles present
    Added ORCID for authors.
  • Runs on local MacOS (silicon M1 chip) only with new environment file
    Updated environment file.
  • If downloading ERA5 data, need to change the pd.date_range end date to “2020-04-30”
    Updated end date.
  • I notice IceNet also has an ORA5Downloader() class. It would be great to include an example of how to use this in the notebook, since the notebook mentions that ORA5 data are used.
  • The anchor links in the Highlights heading do not seem to work
    Fixed links
  • DOIs are missing for the specific OSI-SAF, ERA5 and ORA5 data used in the notebook
    Added citation with DOI for data sources
  • The original U-Net model introduced by Andersson was trained to do classification tasks (predict local monthly-mean sea ice extent), however the model here is refactored to a regression task (predict local daily sea ice concentration). It is unclear whether this new model would have the same predictive skill as the original model over the lead times explored in Andersson et al. Perhaps this is outside the scope of this notebook, but it would be interesting to see if you use the new model to predict an entire month of daily SIC (from which you can get daily SIE and then compute a monthly-mean), does it achieve similar skill to the original U-Net model's prediction to monthly SIE? I appreciate that for computational reasons the model in this notebook is limited in terms of training and inputs, but I think it would be useful to quantify what these limitations have on the prediction skill - especially if someone did want to use this for operational sea ice forecasting.
    This could be an idea to include on planned paper on current state of IceNet, but do agree its probably outside the scope of this submission, so, will omit.
@bnubald bnubald added the enhancement New feature or request label Jun 3, 2024
@bnubald bnubald added this to the v0.1 milestone Jun 3, 2024
@bnubald bnubald self-assigned this Jun 3, 2024
@bnubald bnubald changed the title [REVIEW] Update language/styling based on review feedback [REVIEW] Update language/styling based on reviewer 1's feedback Jun 4, 2024
@bnubald bnubald changed the title [REVIEW] Update language/styling based on reviewer 1's feedback [REVIEW] Update language/styling based on reviewer's feedback Jun 4, 2024
bnubald added a commit that referenced this issue Jun 4, 2024
bnubald added a commit that referenced this issue Jun 7, 2024
bnubald added a commit that referenced this issue Jun 7, 2024
This reverts commit d50a166. Change of incorrect date.
bnubald added a commit that referenced this issue Jun 7, 2024
bnubald added a commit that referenced this issue Jun 7, 2024
bnubald added a commit that referenced this issue Jun 7, 2024
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

No branches or pull requests

1 participant