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

Make a version of the notebooks for the AFR-22 domain #136

Merged
merged 24 commits into from
Sep 27, 2022
Merged

Conversation

nhsavage
Copy link
Collaborator

These are a new set of notebooks, derived from the EAS-22 ones. Some fixes to the notebooks will need to be pushed back to the EAS-22 ones. I will open a separate ticket for these issues.

@nhsavage nhsavage requested a review from gredmond-mo April 29, 2022 08:52
@nhsavage nhsavage requested a review from rosannaamato July 12, 2022 10:43
@rosannaamato
Copy link
Contributor

Branch reverted to Nick's last commit (cc22230) as I have removed my commits that should have been made in a separate branch. Apologies, tidy again.

Copy link
Collaborator

@gredmond-mo gredmond-mo left a comment

Choose a reason for hiding this comment

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

makedata.ipynb

  • fails with permissions errors because the user specific directories do not exist e.g. $SCRATCH/cordex_inputs/future/
  • unused cell at the bottom of the notebook.

worksheet1.ipynb

  • when making symlinks and rsyncing the data it is not clear which directory the user should be in, I assume $DATADIR/data_afr22 for rsyncing the data? but the directory containing the notebooks for the symlinking?
  • the symlink line doesn't seem to work as intended and you end up with: data_afr22 data_afr22 -> $DATADIR/data_afr22 data_afr22 but if you run it directly on the linux command line (taking out the subprocess stuff) then it works fine.
  • the rsync command only copies the AFR_22 data not the CHIRPS or CRU because they are symlinks
  • no data from 2021 present so listing mon_202101-??????.nc fails
  • in the first code block of 1.2 I would recommend explicitly cding into the correct directory (to prevent confusion, even though they should already be in it.)

General comment
I think one of the main possible stumbling blocks is the underlying assumption about which directory the user is in, I would be tempted at least at the start of every worksheet to add in a pwd or similar so users can check they are in the right directory.

@nhsavage
Copy link
Collaborator Author

General comment I think one of the main possible stumbling blocks is the underlying assumption about which directory the user is in, I would be tempted at least at the start of every worksheet to add in a pwd or similar so users can check they are in the right directory.

I agree that this is a stumbling block. However I think it has to be addressed on a different issue. The challenge with it is that the path to use will be platform dependent - so some sort of include file with the platform specific needs to be set up. This would also include potentially scripts fetching from S3 buckets or precisrcm and doing the linking.

@nhsavage
Copy link
Collaborator Author

General comment I think one of the main possible stumbling blocks is the underlying assumption about which directory the user is in, I would be tempted at least at the start of every worksheet to add in a pwd or similar so users can check they are in the right directory.

I agree that this is a stumbling block. However I think it has to be addressed on a different issue. The challenge with it is that the path to use will be platform dependent - so some sort of include file with the platform specific needs to be set up. This would also include potentially scripts fetching from S3 buckets or precisrcm and doing the linking.

On reflection, I will make this work for the existing VDI set up and I have opened #145 to deal with multiple sets of options for download of data

@nhsavage
Copy link
Collaborator Author

Closes #124

@nhsavage
Copy link
Collaborator Author

I have now updated to account for the comments apart from the last one "in the first code block of 1.2 I would recommend explicitly cding into the correct directory (to prevent confusion, even though they should already be in it.)" - I want this to be covered in a follow on ticket

@nhsavage nhsavage merged commit 10485e1 into develop Sep 27, 2022
@nhsavage nhsavage deleted the new/afr_22 branch September 27, 2022 13:18
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.

3 participants