OceanBioME / OceanBioME.jl

🌊 🦠 🌿 A fast and flexible modelling environment written in Julia for modelling the coupled interactions between ocean biogeochemistry, carbonate chemistry, and physics
https://oceanbiome.github.io/OceanBioME.jl/
MIT License
48 stars 21 forks source link

JOSS paper #76

Closed jagoosw closed 1 year ago

jagoosw commented 1 year ago

To do:

jagoosw commented 1 year ago

Docs now fixed and more complete #77

jagoosw commented 1 year ago

Now I've compiled the draft I do think I need to make the figure font sizes larger

johnryantaylor commented 1 year ago

It’s mostly the font sizes that are too small to easily read at 100% magnification.  However, the way that the sub panels are arrange might make it difficult to increase the font size.  It might be better to have all sub panels in a single column.  Another small comment is that the time for the Kelp Growth in Figure 1 might be confusing since the axis label starts at t=0, but the kelp don’t actually start growing until near the end of the second year.

On Mar 18, 2023 at 7:56 PM +0000, Jago Strong-Wright @.***>, wrote:

Now I've compiled the draft I do think I need to make the figure font sizes larger — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you are subscribed to this thread.Message ID: @.***>

jagoosw commented 1 year ago

@johnryantaylor For the column figure, do you think this is large enough font now? Or should I move the panels around?

https://github.com/OceanBioME/OceanBioME.jl/suites/11702149339/artifacts/608761660

johnryantaylor commented 1 year ago

The new Figure 1 looks great, thanks!  Fonts are still small in Figure 2 (but I think you haven’t updated that one). On Mar 21, 2023 at 11:23 AM +0000, Jago Strong-Wright @.***>, wrote:

For the column figure, do you think this is large enough font now? Or should I move the panels around? https://github.com/OceanBioME/OceanBioME.jl/suites/11702149339/artifacts/608761660 — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you commented.Message ID: @.***>

jagoosw commented 1 year ago

@johnryantaylor are you happy with how it is now? It does mean the colorbars are quite big but I don't think theres any other way to include them, and do think they need to be included.

https://github.com/OceanBioME/OceanBioME.jl/suites/11715754847/artifacts/609710562

johnryantaylor commented 1 year ago

Looks great, thanks! On Mar 21, 2023 at 8:15 PM +0000, Jago Strong-Wright @.***>, wrote:

@johnryantaylor are you happy with how it is now? It does mean the colorbars are quite big but I don't think theres any other way to include them, and do think they need to be included. https://github.com/OceanBioME/OceanBioME.jl/suites/11715754847/artifacts/609710562 — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: @.***>

jagoosw commented 1 year ago

Made a mess trying to merge main

codecov[bot] commented 1 year ago

Codecov Report

All modified lines are covered by tests :white_check_mark:

Comparison is base (4cf6565) 65.78% compared to head (7e92e74) 65.78%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #76 +/- ## ======================================= Coverage 65.78% 65.78% ======================================= Files 27 27 Lines 1064 1064 ======================================= Hits 700 700 Misses 364 364 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

jagoosw commented 1 year ago

Just realised the column figure is slightly wrong still

jagoosw commented 1 year ago

It's automatically adding an affiliation to "Collaborators", given that I think everyone else who has helped is affiliated with CliMA we could put that?

johnryantaylor commented 1 year ago

I noticed that too, but I don’t think that it matters right now - we can update as we add more names. On Jun 9, 2023 at 2:26 PM +0100, Jago Strong-Wright @.***>, wrote:

It's automatically adding an affiliation to "Collaborators", given that I think everyone else who has helped is affiliated with CliMA we could put that? — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: @.***>

jagoosw commented 1 year ago

@johnryantaylor I've tried adding the suggestions from @glwagner. Not sure I've added them in the best place?

glwagner commented 1 year ago

I made a comment! Note, it's easier to comment on the markdown file if each sentence has it's own line.

jagoosw commented 1 year ago

I made a comment! Note, it's easier to comment on the markdown file if each sentence has it's own line.

Good to know, I'll add some line breaks to make it easier. Thanks!

jagoosw commented 1 year ago

Do we want British or American English? One of the spellings Grammarly caught was an American spelling but the rest are the British versions I think. The JOSS documentation doesn't seem to express a preference.

jagoosw commented 1 year ago

hi @jagoosw and @johnryantaylor, paper reads well! -- just a few minor comments. Nothing crucial, take them or leave them.

Thank you! These are very helpful

navidcy commented 1 year ago

Another remark is that sometimes we refer to packages using their .jl ending and sometimes not. Should we homogenize or is it clear you reckon?

jagoosw commented 1 year ago

Another remark is that sometimes we refer to packages using their .jl ending and sometimes not. Should we homogenize or is it clear you reckon?

This is a good point, I think it flows better to not have the .jl but other paper do consistently use the whole name in ` marks.

jagoosw commented 1 year ago

I've changed them now but it might look a bit funny so we might want to change them all back

navidcy commented 1 year ago

This looks pretty much ready!

We can include a Project.toml and Manifest.toml file in the paper directory so that the scripts that reproduce the figures are ensured to run. But we don't need to do this now in case we ask to revise the figures in the review process.

jagoosw commented 1 year ago

This looks pretty much ready!

We can include a Project.toml and Manifest.toml file in the paper directory so that the scripts that reproduce the figures are ensured to run. But we don't need to do this now in case we ask to revise the figures in the review process.

Great, thanks!

jagoosw commented 1 year ago

@johnryantaylor @syou83syou83 @glwagner should I submit this now if everyone's happy?

simone-silvestri commented 1 year ago

Hello @jagoosw, thanks for the addition. Good job with the paper; it reads very nicely. Also, good job with the package! I also have a couple of minor comments, take a look if you'd like.

jagoosw commented 1 year ago

Hi @simone-silvestri, thank you, these are very useful comments!

When we wrote about PISCES we were planning on implementing it, and I've done some of the work to do so in a branch that is very out of date now, so I will remove that from the list.

jagoosw commented 1 year ago

Hi all, I've attempted to make some of the changes suggested in the review for the paper here. Not sure it's all correct yet but hopefully should satisfy the queries. I'm also going to open a PR with the other docs changes.

jagoosw commented 1 year ago

I've tried to add a couple of sentences and a short code snippet about how easy it is to modify models, but I'm not sure if I've done it quite clumsily.

Francis has made a good point that this is an important feature that we should definitely note.

jagoosw commented 1 year ago

Okay, so I've tried rewriting the bit about adding models referencing the docs I've written #131. I'm not sure it's right still, thoughts would be appreciated.

jagoosw commented 1 year ago

I think this is now finalised if everyone is happy? @johnryantaylor

navidcy commented 1 year ago

I'm happy. Could you perhaps resolve the conflicts in simple_multi_G.jl?

jagoosw commented 1 year ago

I'm happy. Could you perhaps resolve the conflicts in simple_multi_G.jl?

done

jagoosw commented 1 year ago

I might have deleted a change you made to the sediment thing actually @navidcy ?

jagoosw commented 1 year ago

Once we're done we need to delete the draft workflow before merging too