peak_finder #64

2020-09-10T15:15:08+02:00

usov_i commented

2020-09-10 15:15:08 +02:00

(Migrated from gitlab.psi.ch)

Created by: JakHolzer

function to find peaks (more detailed description in email)

*Created by: JakHolzer* function to find peaks (more detailed description in email)

usov_i commented

2020-09-11 10:58:10 +02:00

(Migrated from gitlab.psi.ch)

Created by: ivan-usov

Normally, we try to keep the code "to the left", meaning lower number of indentations is better. Here you can check for a failed condition and do the early exit (guard clause), e.g. instead of

if condition:
    # your code
else:
    # error
    return

do

if not condition:
   # error
   return

# your code

*Created by: ivan-usov* Normally, we try to keep the code "to the left", meaning lower number of indentations is better. Here you can check for a failed condition and do the early exit (guard clause), e.g. instead of ``` if condition: # your code else: # error return ``` do ``` if not condition: # error return # your code ```

usov_i commented

2020-09-11 10:59:42 +02:00

(Migrated from gitlab.psi.ch)

Created by: ivan-usov

Review: Changes requested

Try to push the change to the same branch (JakHolzer-peak-finder) and the changes will appear here. Then we can merge!

*Created by: ivan-usov* **Review:** Changes requested Try to push the change to the same branch (JakHolzer-peak-finder) and the changes will appear here. Then we can merge!

usov_i commented

2020-09-11 15:31:14 +02:00

(Migrated from gitlab.psi.ch)

Created by: JakHolzer

done 👍

*Created by: JakHolzer* done 👍

usov_i commented

2020-09-11 15:40:10 +02:00

(Migrated from gitlab.psi.ch)

Created by: ivan-usov

You can simply replace the default values of function's optional arguments with those numbers.

*Created by: ivan-usov* You can simply replace the default values of function's optional arguments with those numbers.

usov_i commented

2020-09-11 15:43:31 +02:00

(Migrated from gitlab.psi.ch)

Created by: ivan-usov

If you consider that the parameter is invalid, then raise ValueError(). The function should be aborted, rather than continued.

*Created by: ivan-usov* If you consider that the parameter is invalid, then `raise ValueError()`. The function should be aborted, rather than continued.

usov_i commented

2020-09-11 16:43:08 +02:00

(Migrated from gitlab.psi.ch)

Created by: JakHolzer

Well I though it would be better this way - if the value is wrong, than default is selected instead, function can run and user knows that the input was wrong. I understand, that GUI will not have output for a user to see, but the line can be easily changed to fit your needs. I was thinking that instead of print('value is wrong, set to xx'), you could put some messagebox with similar comment. Let me know what you think, I can change it.

*Created by: JakHolzer* Well I though it would be better this way - if the value is wrong, than default is selected instead, function can run and user knows that the input was wrong. I understand, that GUI will not have output for a user to see, but the line can be easily changed to fit your needs. I was thinking that instead of print('value is wrong, set to xx'), you could put some messagebox with similar comment. Let me know what you think, I can change it.

usov_i commented

2020-09-11 16:46:05 +02:00

(Migrated from gitlab.psi.ch)

Created by: JakHolzer

Thank you, that really does look much better.

*Created by: JakHolzer* Thank you, that really does look much better.

usov_i commented

2020-09-11 17:46:26 +02:00

(Migrated from gitlab.psi.ch)

Created by: ivan-usov

I see now, well, I don't have a strong opinion on changing bad parameters to their defaults, but I still have a few comments:

Default value for int_threshold is 0.8 in the function and 0.75 in the check.
It's clearer to check for a negative condition, rather than pass on a positive one and do the work inside else.

*Created by: ivan-usov* I see now, well, I don't have a strong opinion on changing bad parameters to their defaults, but I still have a few comments: * Default value for `int_threshold` is 0.8 in the function and 0.75 in the check. * It's clearer to check for a negative condition, rather than `pass` on a positive one and do the work inside `else`.

usov_i commented

2020-09-14 09:43:54 +02:00

(Migrated from gitlab.psi.ch)

Created by: JakHolzer

Both corrected. I must admit that now, when I can see the original code and the new one, I'm starting to see a bit the "programming" thinking. Thank you!

*Created by: JakHolzer* Both corrected. I must admit that now, when I can see the original code and the new one, I'm starting to see a bit the "programming" thinking. Thank you!

usov_i commented

2020-09-15 14:25:40 +02:00

(Migrated from gitlab.psi.ch)

Created by: ivan-usov

Review: Approved

*Created by: ivan-usov* **Review:** Approved

usov_i commented

2020-09-15 14:26:01 +02:00

(Migrated from gitlab.psi.ch)

Merged by: ivan-usov at 2020-09-15 12:26:01 UTC

*Merged by: ivan-usov at 2020-09-15 12:26:01 UTC*

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: zebra/pyzebra#64