Fix error when setting outplanting levels due to latest RME expecting density as vector #29

Rosejoycrocker · 2024-12-06T05:39:37Z

Resolves #28

…ved in v1.0.31

If version< 1.0.28 then expect Int, if not expect vector

…patibility

ConnectedSystems · 2024-12-06T06:31:19Z

src/deployment.jl

+function version_check()::Vector{Int64}
+    rme_ver = @RME version()::Cstring
+    return parse.(Int64, split(rme_ver, '.'))
+end


I have an issue with the function name in that it doesn't perform a "check".

To me, it should be rme_version() or similar as it simply returns the version number (albeit separated).
Simply version_check can be confusing. It's unclear at first glance: What does it mean? Version of the Julia package?

I'm also hesitant to accept it returning a vector of integers. Version numbers should be treated as IDs firstly.

I also dislike how this is being used. Remember that if you're hardcoding index values, then it's usually a smell telling you there's a better way.

In this case, I would prefer:

a named tuple be returned: (major=1, minor=0, patch=28) (following SemVer)

the function name be changed to rme_version_info()

ConnectedSystems · 2024-12-06T06:34:09Z

src/deployment.jl

+    Set outplanting deployments across a range of years, automatically determining the
 coral deployment density to maintain the set grid size.


No indent for main text in docstrings

ConnectedSystems · 2024-12-06T06:34:54Z

src/deployment.jl

    req_area = area_needed(max_n_corals, density)

    # Divide by half (i.e., `* 0.5`) as RME simulates two deployments per year
-    mod_density = (n_corals * 0.5) / (req_area / m2_TO_km2)
-    deployment_area_pct = min((req_area / sum(target_areas)) * 100.0, 100.0)
+    mod_density = (n_corals * 0.5) ./ (req_area ./ m2_TO_km2)


Is this line a duplication of area_needed()?

ConnectedSystems · 2024-12-06T06:36:00Z

src/deployment.jl

-    mod_density = (n_corals * 0.5) / (req_area / m2_TO_km2)
-    deployment_area_pct = min((req_area / sum(target_areas)) * 100.0, 100.0)
+    mod_density = (n_corals * 0.5) ./ (req_area ./ m2_TO_km2)
+    deployment_area_pct = min((sum(req_area) / sum(target_areas)) * 100.0, 100.0)


Calculate sum(req_area) once beforehand and replace the repeated sum() here and below please.

ConnectedSystems · 2024-12-06T06:37:21Z

src/deployment.jl

    deployment_area_pct = min((req_area / sum(target_areas)) * 100.0, 100.0)

+    # In RME versions higher than 1.0.28 density needs to be a vector with each element representing density per species
+    version_vec = version_check()
+    density = (version_vec[3] <= 28) && (version_vec[1] == 1) ? density : fill(density, 6)


Ignoring the hardcoded indexing here, think about what would happen if the RME version advances to v1.2.28

ConnectedSystems · 2024-12-06T06:37:43Z

src/deployment.jl

@@ -130,15 +156,30 @@ function set_outplant_deployment!(
    last_year::Int64,
    year_step::Int64,
    area_km2::Vector{Float64},
-    density::Float64
+    density::Vector{Float64}


Doesn't this break backward compatibility?

ConnectedSystems · 2024-12-06T06:39:33Z

docs/src/getting_started.md

-# Add 3 DHW enhancement to outplanted corals
-set_option("restoration_dhw_tolerance_outplants", 3)
-


Could you clarify why the outplanting is removed in the example?

The commit message for this change just says "fix docs"?

restoration_dhw_tolerance_outplants was removed in version 1.0.31

Are we dropping backwards compatibility?

Check update to include major, minor and patch

Define sum(area_req) as variable to avoid multiple summations Fix docs

Rosejoycrocker · 2024-12-09T01:52:04Z

I believe I've addressed your comments @ConnectedSystems

ConnectedSystems · 2024-12-09T23:47:08Z

src/deployment.jl

+    rme_ver = @RME version()::Cstring
+    rme_ver = parse.(Int64, split(rme_ver, '.'))


No change required here, just noting this is type unstable (going from string to vector of ints) so might need an adjustment later.

ConnectedSystems · 2024-12-09T23:49:48Z

src/deployment.jl

+- `density` : Stocking density per m². In RME versions higher than 1.0.28 density needs to be a vector
+    with each element representing density per species


Suggested change

- `density` : Stocking density per m². In RME versions higher than 1.0.28 density needs to be a vector

with each element representing density per species

- `density` : Stocking density per m². In RME versions higher than v1.0.28, density is expected to be a vector

with each element representing the density per functional group

ConnectedSystems · 2024-12-09T23:51:36Z

src/deployment.jl

-    @RME ivSetOutplantCountPerM2(name::Cstring, mod_density::Cdouble)::Cint
+
+    rme_version = rme_version_info()
+    if !((rme_version.patch <= 28) && (rme_version.minor == 0) && (rme_version.major == 1))


Reorder to be in logical order please: major, minor, patch.

Rosejoycrocker · 2024-12-10T03:08:56Z

I've addressed your comments @ConnectedSystems, let me know if there are others

ConnectedSystems · 2024-12-10T06:35:11Z

@Rosejoycrocker there's still the question of what to do with the example currently in the docs.

It would be best to show how to do intervention runs with the most recent version of RME we have at the very least, with a note on how to do it with older versions.

Rosejoycrocker · 2024-12-11T23:08:52Z

@Rosejoycrocker there's still the question of what to do with the example currently in the docs.

It would be best to show how to do intervention runs with the most recent version of RME we have at the very least, with a note on how to do it with older versions.

I'll add the removed example as a comment with a note about the version

…was removed in v1.0.31" This reverts commit a42a1c6.

… on versions

ConnectedSystems · 2024-12-12T04:24:18Z

docs/src/getting_started.md

+# Adding dhw tolerance was removed in v1.0.31
+# Add 3 DHW enhancement to outplanted corals
+# set_option("restoration_dhw_tolerance_outplants", 3)
+


You need to demonstrate what to do with versions > v1.0.31

From the docs and matlab files it looks like they added an rme_iv_set_outplant_heat_tolerance function.

The relevant RME functions are:

ivSetOutplantHeatToleranceMeanDhw and ivSetOutplantHeatToleranceSdDhw

Both of which expect a vector of values for each functional group.

Actually, I'm going to accept and merge this as is and address the above later.

Rosejoycrocker added 5 commits December 6, 2024 15:22

Remove reference to "restoration_dhw_tolerance_outplants" as was remo…

a42a1c6

…ved in v1.0.31

Add version check

b7bd1c5

If version< 1.0.28 then expect Int, if not expect vector

Add Union{Float64, Vector{Float64}} output types to allow version com…

d9f547a

…patibility

Fix docs and update area_needed for version compatibility

2120a33

Formatting

2cdcc99

Rosejoycrocker requested a review from DanTanAtAims December 6, 2024 05:39

Rosejoycrocker added the bug Something isn't working label Dec 6, 2024

Fold version checking into function

fe27493

ConnectedSystems requested changes Dec 6, 2024

View reviewed changes

Rosejoycrocker added 3 commits December 9, 2024 11:21

Update version retrieval to NamedTuple output

c6bfe1a

Check update to include major, minor and patch

Correct type of density for version compatibility

f4eb6a2

Correct formatting and syntax

b1d80d0

Define sum(area_req) as variable to avoid multiple summations Fix docs

Rosejoycrocker force-pushed the fix-outplant-function branch from 26f3418 to b1d80d0 Compare December 9, 2024 01:44

ConnectedSystems reviewed Dec 9, 2024

View reviewed changes

Minor docs fixes and ordering of version checking statements

db05532

Rosejoycrocker added 2 commits December 12, 2024 09:10

Revert "Remove reference to "restoration_dhw_tolerance_outplants" as …

241318d

…was removed in v1.0.31" This reverts commit a42a1c6.

Add example of setting dhw tolerance back in but as comment with note…

4256add

… on versions

ConnectedSystems reviewed Dec 12, 2024

View reviewed changes

ConnectedSystems approved these changes Dec 12, 2024

View reviewed changes

ConnectedSystems merged commit a6d3cfa into main Dec 12, 2024
1 check passed

ConnectedSystems deleted the fix-outplant-function branch December 12, 2024 04:24

ConnectedSystems mentioned this pull request Dec 12, 2024

Demonstrate how to set heat tolerance for deployments #31

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix error when setting outplanting levels due to latest RME expecting density as vector #29

Fix error when setting outplanting levels due to latest RME expecting density as vector #29

Rosejoycrocker commented Dec 6, 2024

ConnectedSystems Dec 6, 2024

ConnectedSystems Dec 6, 2024

ConnectedSystems Dec 6, 2024

ConnectedSystems Dec 6, 2024

ConnectedSystems Dec 6, 2024

ConnectedSystems Dec 6, 2024

ConnectedSystems Dec 6, 2024 •

edited

Loading

DanTanAtAims Dec 9, 2024

ConnectedSystems Dec 9, 2024 •

edited

Loading

Rosejoycrocker commented Dec 9, 2024

ConnectedSystems Dec 9, 2024 •

edited

Loading

ConnectedSystems Dec 9, 2024

ConnectedSystems Dec 9, 2024

Rosejoycrocker commented Dec 10, 2024

ConnectedSystems commented Dec 10, 2024 •

edited

Loading

Rosejoycrocker commented Dec 11, 2024

ConnectedSystems Dec 12, 2024 •

edited

Loading

		Set outplanting deployments across a range of years, automatically determining the
		coral deployment density to maintain the set grid size.

		# Add 3 DHW enhancement to outplanted corals
		set_option("restoration_dhw_tolerance_outplants", 3)

		rme_ver = @RME version()::Cstring
		rme_ver = parse.(Int64, split(rme_ver, '.'))

		- `density` : Stocking density per m². In RME versions higher than 1.0.28 density needs to be a vector
		with each element representing density per species

Fix error when setting outplanting levels due to latest RME expecting density as vector #29

Fix error when setting outplanting levels due to latest RME expecting density as vector #29

Conversation

Rosejoycrocker commented Dec 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ConnectedSystems Dec 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ConnectedSystems Dec 9, 2024 • edited Loading

Choose a reason for hiding this comment

Rosejoycrocker commented Dec 9, 2024

ConnectedSystems Dec 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rosejoycrocker commented Dec 10, 2024

ConnectedSystems commented Dec 10, 2024 • edited Loading

Rosejoycrocker commented Dec 11, 2024

ConnectedSystems Dec 12, 2024 • edited Loading

Choose a reason for hiding this comment

ConnectedSystems Dec 6, 2024 •

edited

Loading

ConnectedSystems Dec 9, 2024 •

edited

Loading

ConnectedSystems Dec 9, 2024 •

edited

Loading

ConnectedSystems commented Dec 10, 2024 •

edited

Loading

ConnectedSystems Dec 12, 2024 •

edited

Loading