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

Fix readjustment of number of slices bug in RSA #679

Merged
merged 23 commits into from
Feb 14, 2024
Merged

Fix readjustment of number of slices bug in RSA #679

merged 23 commits into from
Feb 14, 2024

Conversation

Rosejoycrocker
Copy link
Collaborator

Fixes readjustment of number of slices bug in RSA (partially addresses #615) and changes output of RSA to YAXArrays.Dataset type.

@Rosejoycrocker Rosejoycrocker mentioned this pull request Feb 11, 2024
Closed
@Rosejoycrocker Rosejoycrocker self-assigned this Feb 12, 2024
@ConnectedSystems
Copy link
Collaborator

Does the switch to YAXArray break the ability to plot results?
I don't see any compatibility layer in place.

@Rosejoycrocker
Copy link
Collaborator Author

Does the switch to YAXArray break the ability to plot results? I don't see any compatibility layer in place.

There are also changes to viz.rsa() in this PR so plotting works

ConnectedSystems
ConnectedSystems reviewed Feb 13, 2024
View reviewed changes
ext/AvizExt/viz/viz.jl Outdated
@@ -1,7 +1,7 @@
using Makie, DataFrames

using ADRIA: ResultSet, metrics.metric_label, analysis.col_normalize, model_spec
using NamedDims, AxisKeys
using NamedDims, AxisKeys, YAXArrays, DimensionalData
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the import of DimensionalData simply to have the At method?

If so, I'd prefer it be imported via YAXArrays:

import YAXArrays.DD: At

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think I just took this from the YAXArray docs and forgot to remove, I've removed it now

ConnectedSystems
ConnectedSystems reviewed Feb 13, 2024
View reviewed changes
src/analysis/sensitivity.jl Outdated
Comment on lines 9 to 10
YAXArrays,
DimensionalData
Copy link
Collaborator

Choose a reason for hiding this comment

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

As with previous comment - if this is for At, import it directly.

@ConnectedSystems
Copy link
Collaborator

Does the switch to YAXArray break the ability to plot results? I don't see any compatibility layer in place.

There are also changes to viz.rsa() in this PR so plotting works

Thanks Rose, clearly I'm going blind.

ConnectedSystems
ConnectedSystems reviewed Feb 13, 2024
View reviewed changes
ext/AvizExt/viz/sensitivity.jl Outdated
@@ -175,7 +175,7 @@ Makie figure
function ADRIA.viz.rsa!(
g::Union{GridLayout,GridPosition},
rs::ResultSet,
si::NamedDimsArray,
si::YAXArrays.Dataset,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not import Dataset directly so prefacing it with YAXArrays is not necessary?

Or is there a different Dataset type that is causing a conflict?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've replaced with Dataset

ConnectedSystems
ConnectedSystems reviewed Feb 13, 2024
View reviewed changes
Copy link
Collaborator

@ConnectedSystems ConnectedSystems left a comment

Choose a reason for hiding this comment

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

Some minor comments for consideration

@Rosejoycrocker
Adjust definition of S in RSA so that maximum S is used to define the…
477107b 
… storage size, but then S i readjusted according to the factor type
@Rosejoycrocker
Apply same changes for outcome_map, so that input S is still used f…
6ecc98b 
…or factors which are not categorical
@Rosejoycrocker
Make output of rsa Dict with entries for each factor requested
bdabeac 
Add `factors` as input to specify which factors to output
@Rosejoycrocker
Add normalize! import to allow normaliation of rsa sensitivities wh…
20b22e0 
…ile in for loop
@Rosejoycrocker
Change category_bins to only output number of categories
77ef7b1 
Not max between number of categories and default S
@Rosejoycrocker
Find bin sequence for each factor if unordered categorical and store,…
a82a9dd 
… otherwise set as default
@Rosejoycrocker
Loop through factors and if unordered cat, use specific bin set, othe…
53fe67e 
…rwise use defauilt
@Rosejoycrocker
Store in dictionary (r_s) with factor names as keys.
f691df2 
Each entry has bins as first column, sensitivities as second
@Rosejoycrocker
Change name of X_di to X_i for clarity
92c7500
@Rosejoycrocker
Change output types for rsa associated functions
ca925cd
@Rosejoycrocker
Add YAXArrays imports and change output type to YAXArrays dataset
fd8a1c6
@Rosejoycrocker
Remove unused enumerate when calculating bin sequences
7d06990 
Also remove original Dict() storage for sensitivities
@Rosejoycrocker
Add creation of NampleTuple storage for sensitivity values
f8ee245 
Values to be stored in YAXArrays with shared dimensions where relevant  (variables other than unordered categorical type)
Keys of NamedTuple are variable names
@Rosejoycrocker
Changes to r_s indexing due to being changed from a Dict to a NamedTuple
17ba236
@Rosejoycrocker
Update formatting
bf9ef26
@Rosejoycrocker
Add additional bin for unordered categoricals for correct binnings
19d75f1
@Rosejoycrocker
Add YAXArrays.Dataset as input to viz.rsa in place of NamedDimsArray
2ac607f
@Rosejoycrocker
Changes to access bins and sensitivities to plot from YAXArrays
6362c2d
@Rosejoycrocker
Add YAXArrays import to viz.jl
aeddfc5
@Rosejoycrocker Rosejoycrocker force-pushed the fix-rsa-bug-in-slice-labels branch from 2ff9c61 to 3324de3 Compare February 13, 2024 06:21
@Rosejoycrocker
Copy link
Collaborator Author

Some minor comments for consideration

I think I've addressed your comments, If you're happy with this I'll do another PR with similar changes for outcome_map

@ConnectedSystems
Copy link
Collaborator

Busy with something else at the moment, would you mind attaching a figure here to demonstrate its working please?

@Rosejoycrocker
Copy link
Collaborator Author

Rosejoycrocker commented Feb 14, 2024 •
edited
Loading

Busy with something else at the moment, would you mind attaching a figure here to demonstrate its working please?

Here's an example
sensitivity_rsa_ex

I'm not sure why there's the peak for guided and the number of corals, it's only 1024 runs

@ConnectedSystems
Copy link
Collaborator

The peak is telling you that there is high variability in that factor space.

In other words, the outcome assessed is highly sensitive to that region of factor space, which corresponds with no intervention.

@Rosejoycrocker
Copy link
Collaborator Author

The peak is telling you that there is high variability in that factor space.

In other words, the outcome assessed is highly sensitive to that region of factor space, which corresponds with no intervention.

Makes sense

ConnectedSystems
ConnectedSystems approved these changes Feb 14, 2024
View reviewed changes
Copy link
Collaborator

@ConnectedSystems ConnectedSystems left a comment

Choose a reason for hiding this comment

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

LGTM

@ConnectedSystems ConnectedSystems merged commit cfeee82 into main Feb 14, 2024
@ConnectedSystems ConnectedSystems deleted the fix-rsa-bug-in-slice-labels branch February 14, 2024 03:43
@DanTanAtAims DanTanAtAims mentioned this pull request Feb 15, 2024
Closed
@DanTanAtAims DanTanAtAims mentioned this pull request Aug 1, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ConnectedSystems ConnectedSystems ConnectedSystems approved these changes

Assignees

@Rosejoycrocker Rosejoycrocker

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Rosejoycrocker @ConnectedSystems