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.

Sign up for GitHub

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

Jump to bottom

JOSS Review Meta-Response #66

Merged
merged 26 commits into from
Apr 10, 2024
Merged

JOSS Review Meta-Response #66

merged 26 commits into from
Apr 10, 2024

Conversation

crvernon
Copy link
Contributor

Addresses the JOSS paper review responses in: openjournals/joss-reviews#6325

Issues addressed:
Resolves #43
Resolves #44
Resolves #47
Resolves #48
Resolves #49
Resolves #50
Resolves #51
Resolves #52
Resolves #54
Resolves #55
Resolves #56
Resolves #57
Resolves #58
Resolves #59
Resolves #60
Resolves #61
Resolves #62
Resolves #63
Resolves #64

Associated PRs:
#45
#46
#53
#65

@crvernon
add pathlib to address path fail on win
02be24d
@crvernon
Merge pull request #45 from IMMM-SFA/bug/path
6af0e15 
Add pathlib to address path fail on windows
@crvernon
add windows to action build
0a5f2c7
@crvernon
adjustment to implementation of path
cefa98f
@crvernon
Merge pull request #46 from IMMM-SFA/feature/windows-action
39155ef 
Add GitHub action for Windows build
@crvernon
add pre-commit to deps
715427b
@crvernon
pre-commit fixes to code
2c713c4
@crvernon
actions cleanup
4f56f20
@crvernon
add pre-commit badge to readme
0646fe7
@crvernon
pre-commit fix
063f2e5
@crvernon
Merge pull request #53 from IMMM-SFA/feature/pre-commit
d441274 
Add pre-commit
@crvernon
notebook mods
0f84b9f
@crvernon
remove relect notebook
f21595f
@crvernon
limit numpy to <2 for pyarrow
1cf7c79
@crvernon
remove codecov moving to paid plan
55d9e58
@crvernon
pre-commit fixes to nbs
7aea2df
@crvernon
add option for xdd dtype conversion
ec54a41
@crvernon
Merge branch 'feature/notebook-mods' of https://github.com/IMMM-SFA/s…
63d0894 
…tatemodify into feature/notebook-mods
@crvernon
fix call to preserve type fn
569eb51
@crvernon
propagate call to dtypes to fn
6744022
@crvernon
fix field dtypes internal call
7562cba
@crvernon
update dtypes call in notebooks
3ed9f41
@crvernon
format notebooks for docs
f2d72cd
@crvernon
pre-commit formatting fixes
83148e5
@crvernon
Merge pull request #65 from IMMM-SFA/feature/notebook-mods
dbbe0bf 
JOSS Review:  Notebook modifications
@crvernon
update to version 0.2
972b05e
@rg727
Copy link
Collaborator

rg727 commented Apr 10, 2024

Finished addressing the first round of JOSS reviews

@rg727 rg727 closed this Apr 10, 2024
@rg727 rg727 reopened this Apr 10, 2024
@rg727 rg727 merged commit 75d0ba2 into main Apr 10, 2024
9 checks passed
@crvernon crvernon mentioned this pull request Apr 10, 2024
Closed
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rg727 rg727 rg727 approved these changes

Assignees

@rg727 rg727

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

A few public functions do not appear to have docstrings: hmm_multisite_sample, hmm_multisite_fit. I guess there are more. You can find these by pip install ruff, navigate to directory, ruff . --select D100. I don't think you have to go too far in this, as it is probably not the responsibility of to fully train a user in StateMod, but since I think there is an implication that is a portal to new users of StateMod, a little more help explaining its principles in the notebooks would be appreciated. I understand you are all probably experts in it so it is probably difficulty to identify what is implicit and what is not, so I will just list things I would appreciate explaining under this task as I identify them Maybe I am impatient but I wonder if you could use a shorter selection of dates in your quickstart notebooks, perhaps just select an interesting decade (or even better a year!)? 20-30 mins doesn't feel like a particularly quick start. Edit: in fact I wonder if changing the dates would be a helpful statemodify functionality (and the ability to change any of the .ctl parameters at runtime) To avoid potential other issues like this, I think it would be sensible for your testing in automation to run on windows and macos. You would only need to add windows-latest and macos-latest into .github/workflows/build.yml. Edit: having started the notebooks I can see that StateMod is compiled for linux and mingw/cygwin, so I can see why you'd only test ubuntu, that said - considering there was only one error on my windows machine, it probably won't hurt and ensures that potential future versions of StateMod will be compatible. Path error in test, I think due to my using a windows machine. Should be fixable by using os.path.join in COMP_FILE under TestModify. Though you may want to consider using pathlib throughout for a more convenient long term handling of file paths than os.
More generally on this (don't necessarily need to address this for JOSS), implementing linters including such as ruff may be helpful to ensure code quality is maintained. In general I like the largely functional approach that you have taken, but I do wonder if a class might be helpful to wrap these processes together. Particularly the 'iterate over samples' loop in all of the notebooks does somehow feel like it should be contained in a class rather than a user having to copy it over each time. Edit: whether a class, or maybe some kind of configuration file. I think notebook 5 probably needs a bit more explanation. From looking at the source code it seems clear that modify_batch and its sub-functions are doing a lot of heavy lifting, and from looking at the data folder (data/cm2015_StateMod/StateMod/scenarios/S0_1) I guess it is creating these files - but I feel this puts quite a lot on a user to figure out what's going on there. notebook UI sampling docs and unit tests
2 participants
@crvernon @rg727