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

Investigate negative fluxes #26

Merged
merged 2 commits into from
Oct 11, 2023
Merged

Investigate negative fluxes #26

merged 2 commits into from
Oct 11, 2023

Conversation

juanep97
Copy link
Owner

@juanep97 juanep97 commented Oct 10, 2023

In this PR:

  • allow the pipeline to continue after encountering a negative flux by using np.log10 (gives nan) instead of math.log10 (throws error). However this is slower and does not address the origin of this unexpected negative value.
  • for that reasons, leave comments in the source code linking issue Investigate negative flux errors #24 .
  • change hard-coded value by the expression that it comes from, to make it understandable.

Let's keep issue #24 open until we study better where it comes from exactly.

@juanep97 juanep97 force-pushed the investigate_negative_fluxes branch from 5f71d43 to e6fb710 Compare October 10, 2023 23:36
@juanep97 juanep97 requested a review from morcuended October 10, 2023 23:38
@morcuended
Copy link
Collaborator

Even if the pipeline keeps going with calculations, the results in this case are wrong, right? Do you want to allow that? Or simply let the pipeline exit loudly raising an error? In the second way, I would expect these problems to go less unnoticed than if you let the pipeline run "normally".
If this is related to data with whatever problem, there's probably nothing to do with it.

@juanep97
Copy link
Owner Author

These errors or invalid values are probably caused by bad data (we have to make sure, though). If the pipeline continues, and the data become NaNs, they will be automatically flagged. We can still see that there were errors by looking at the error messages. However if the pipeline just stops on the first error, every time there is a single bad point, we will loose whatever data was going to reduce after (worst case, the whole night).

Maybe we could implement an option stop_on_errors that jumps to the interpreted if activated, or just re-raises exceptions instead of continuing with a simple message. But in general it should be resilent to errors so no good data is lost.

@juanep97
Copy link
Owner Author

juanep97 commented Oct 11, 2023

Let's write here whatever instances of this kind of errors we find and debug, so we can make an informed decision about how to treat them. I will add them in the Issue description as they come.

Copy link
Collaborator

@morcuended morcuended left a comment

Choose a reason for hiding this comment

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

These errors or invalid values are probably caused by bad data (we have to make sure, though). If the pipeline continues, and the data become NaNs, they will be automatically flagged. We can still see that there were errors by looking at the error messages. However if the pipeline just stops on the first error, every time there is a single bad point, we will loose whatever data was going to reduce after (worst case, the whole night).
Maybe we could implement an option stop_on_errors that jumps to the interpreted if activated, or just re-raises exceptions instead of continuing with a simple message. But in general it should be resilent to errors so no good data is lost.

Fair enough. Go ahead with the merge

@morcuended
Copy link
Collaborator

Let's write here whatever instances of this kind of errors we find and debug, so we can make an informed decision about how to treat them. I will add them in the Issue description as they come.

Better in the issue #24 than in this PR that will be closed when merging.

@juanep97 juanep97 merged commit 8e32cd6 into main Oct 11, 2023
@juanep97 juanep97 deleted the investigate_negative_fluxes branch October 11, 2023 08:56
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.

2 participants