search

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
40 stars 20 forks source link

Fixed some `show` methods #156

Closed jagoosw closed 10 months ago

jagoosw commented 10 months ago

When I went to update Oceananigans I found these changes I made so thought we might as well merge them first too.

navidcy commented 10 months ago

We dropped showing "Optional components" and "Sinking velocities" from model's show methods?

codecov[bot] commented 10 months ago

Codecov Report

Attention: 10 lines in your changes are missing coverage. Please review.

Comparison is base (37530fc) 65.84% compared to head (1658ddf) 64.18%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #156 +/- ## ========================================== - Coverage 65.84% 64.18% -1.66% ========================================== Files 27 27 Lines 1054 1061 +7 ========================================== - Hits 694 681 -13 - Misses 360 380 +20 ``` | [Files](https://app.codecov.io/gh/OceanBioME/OceanBioME.jl/pull/156?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=OceanBioME) | Coverage Ξ” | | |---|---|---| | [src/Light/2band.jl](https://app.codecov.io/gh/OceanBioME/OceanBioME.jl/pull/156?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=OceanBioME#diff-c3JjL0xpZ2h0LzJiYW5kLmps) | `88.57% <ΓΈ> (ΓΈ)` | | | [src/Models/AdvectedPopulations/NPZD.jl](https://app.codecov.io/gh/OceanBioME/OceanBioME.jl/pull/156?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=OceanBioME#diff-c3JjL01vZGVscy9BZHZlY3RlZFBvcHVsYXRpb25zL05QWkQuamw=) | `87.65% <50.00%> (-1.24%)` | :arrow_down: | | [src/OceanBioME.jl](https://app.codecov.io/gh/OceanBioME/OceanBioME.jl/pull/156?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=OceanBioME#diff-c3JjL09jZWFuQmlvTUUuamw=) | `68.75% <0.00%> (-2.22%)` | :arrow_down: | | [src/Utils/negative\_tracers.jl](https://app.codecov.io/gh/OceanBioME/OceanBioME.jl/pull/156?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=OceanBioME#diff-c3JjL1V0aWxzL25lZ2F0aXZlX3RyYWNlcnMuamw=) | `70.96% <0.00%> (-4.90%)` | :arrow_down: | | [src/Models/AdvectedPopulations/LOBSTER/LOBSTER.jl](https://app.codecov.io/gh/OceanBioME/OceanBioME.jl/pull/156?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=OceanBioME#diff-c3JjL01vZGVscy9BZHZlY3RlZFBvcHVsYXRpb25zL0xPQlNURVIvTE9CU1RFUi5qbA==) | `70.49% <14.28%> (-6.71%)` | :arrow_down: | ... and [1 file with indirect coverage changes](https://app.codecov.io/gh/OceanBioME/OceanBioME.jl/pull/156/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=OceanBioME)

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

jagoosw commented 10 months ago

We dropped showing "Optional components" and "Sinking velocities" from model's show methods?

Where is this? It still shows them here: https://github.com/OceanBioME/OceanBioME.jl/blob/c6e098f211a4765d50b3a4b1c110a248c9a3a619/src/Models/AdvectedPopulations/LOBSTER/LOBSTER.jl#L430-L435

navidcy commented 10 months ago

E.g., look at the changes in docs/src/model_components/biogeochemical/LOBSTER.md I had to do so that the doctests pass. That's what I meant.

jagoosw commented 10 months ago

Ah I see, that's because there LOBSTER is wrapped in Biogeochemistry so its only showing the summary of the underlying model:

julia> bgc_model
Lodyc-DAMTP Ocean Biogeochemical Simulation Tools for Ecosystem and Resources (LOBSTER) model (Float64) 
 Light attenuation: Two-band light attenuation model (Float64)
 Sediment: Nothing
 Particles: Nothing

julia> bgc_model.underlying_biogeochemistry
Lodyc-DAMTP Ocean Biogeochemical Simulation Tools for Ecosystem and Resources (LOBSTER) model (Float64) 
 Optional components:
    β”œβ”€β”€ Carbonates βœ… 
    β”œβ”€β”€ Oxygen ❌ 
    └── Variable Redfield Ratio ❌
 Sinking Velocities:
    β”œβ”€β”€ sPOM: 0.0 to -3.47e-5 m/s 
    └── bPOM: 0.0 to -0.0023148148148148147 m/s

Perhaps the summary should include slightly more information? I could change it so the summary includes:

summary(::LOBSTER{FT, Val{B}, W}) where {FT, B, W} = string("Lodyc-DAMTP Ocean Biogeochemical Simulation Tools for Ecosystem and Resources (LOBSTER) model ($FT)\n Optional components: carbonates $(B[1] ? :βœ… : :❌), oxygen $(B[2] ? :βœ… : :❌), variable Redfield ratio $(B[3] ? :βœ… : :❌)")

But then it is quite long. I don't think we really need the sinking velocities in the summary since the details are a lot less important.

navidcy commented 10 months ago

I think it’s fine. Just wanted to ensure it wasn’t by accident.

Merge at your convenience.

jagoosw commented 10 months ago

I think it’s fine. Just wanted to ensure it wasn’t by accident.

Merge at your convenience.

Ah thank you, just saw this and have changed them a bit so will need to update docstrings