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.

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

Fixing saturation indicator bug #123

Merged
merged 15 commits into from
Aug 12, 2021
Merged

Fixing saturation indicator bug #123

merged 15 commits into from
Aug 12, 2021

Conversation

hn437
Copy link
Contributor

@hn437 hn437 commented Aug 6, 2021

Prevent the mapping saturation indicator from returning nan-Values as result if data is available. See Issue #45 for bug description

Corresponding issue

Closes #45

New or changed dependencies

Checklist

  • I have updated my branch to main (e.g. through git rebase main)
  • My code follows the style guide and was checked with pre-commit before committing
  • I have commented my code
  • I have added sufficient unit and integration tests
  • I have updated the CHANGELOG.md

…alse error-calculation resulting in returning nan-values in the result of the mapping saturation index instead of choosing another calculation parameter
@hn437
Copy link
Contributor Author

hn437 commented Aug 6, 2021

Change to prevent the bug at the moment: Instead of checking a single parameter for nan-values, check all values within the 4 parameters for nan-values and use else-clause if nan-values do exist

@hn437 hn437 requested a review from Hagellach37 August 6, 2021 11:53
@matthiasschaub matthiasschaub requested review from matthiasschaub and removed request for Hagellach37 August 9, 2021 08:47
@joker234
Copy link
Member

joker234 commented Aug 9, 2021

Is inf a valid outcome we want to allow? If not you could reduce your check to something like this:

all(map(math.isfinite, a))

with a as your input list

@hn437
Copy link
Contributor Author

hn437 commented Aug 9, 2021

Is inf a valid outcome we want to allow? If not you could reduce your check to something like this:

all(map(math.isfinite, a))

with a as your input list

no, but you need to check 2 lists and 2 floats which are all written in one tuple. therefore you need to handle the first to elements differently than the last two. and you need to check the results for each element within the nested list as well as the other float elements in the tuple

@hn437
Copy link
Contributor Author

hn437 commented Aug 9, 2021

Is inf a valid outcome we want to allow? If not you could reduce your check to something like this:

all(map(math.isfinite, a))

with a as your input list

no, but you need to check 2 lists and 2 floats which are all written in one tuple. therefore you need to handle the first to elements differently than the last two. and you need to check the results for each element within the nested list as well as the other float elements in the tuple

so you could replace this:
validity_result = all( [ not x for x in map( math.isnan, [ math.fsum( inits5curves[0] ), math.fsum(inits5curves[1]), inits5curves[2], inits5curves[3], ], ) ] )
with this
validity_result = all( [ all(map(math.isfinite, inits5curves[0])), all(map(math.isfinite, inits5curves[1])), math.isfinite(inits5curves[2]), math.isfinite(inits5curves[3]), ] )

is this prefered?

@joker234
Copy link
Member

joker234 commented Aug 9, 2021


so you could replace this:
validity_result = all( [ not x for x in map( math.isnan, [ math.fsum( inits5curves[0] ), math.fsum(inits5curves[1]), inits5curves[2], inits5curves[3], ], ) ] )
with this
validity_result = all( [ all(map(math.isfinite, inits5curves[0])), all(map(math.isfinite, inits5curves[1])), math.isfinite(inits5curves[2]), math.isfinite(inits5curves[3]), ] )

is this prefered?

I meant something like this, it's just a little bit shorter and you don't have to use that much nesting layers ;)

validity_result = all(map(math.isfinite, [*inits5curves[0] *inits5curves[1], inits5curves[2], inits5curves[3]]))

@hn437
Copy link
Contributor Author

hn437 commented Aug 9, 2021


so you could replace this:
validity_result = all( [ not x for x in map( math.isnan, [ math.fsum( inits5curves[0] ), math.fsum(inits5curves[1]), inits5curves[2], inits5curves[3], ], ) ] )
with this
validity_result = all( [ all(map(math.isfinite, inits5curves[0])), all(map(math.isfinite, inits5curves[1])), math.isfinite(inits5curves[2]), math.isfinite(inits5curves[3]), ] )
is this prefered?

I meant something like this, it's just a little bit shorter and you don't have to use that much nesting layers ;)

validity_result = all(map(math.isfinite, [*inits5curves[0] *inits5curves[1], inits5curves[2], inits5curves[3]]))

Okay, thank you! Didn't knew you could use the * like that... implemented it!

@hn437 hn437 merged commit 8221067 into main Aug 12, 2021
@hn437 hn437 deleted the fixing-saturation-indicator-bug branch August 12, 2021 08:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Different None types calculating the mapping saturation indicator
3 participants