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

Fix/128 #129

Merged
merged 3 commits into from
Feb 18, 2019
Merged

Fix/128 #129

merged 3 commits into from
Feb 18, 2019

Conversation

hombit
Copy link
Collaborator

@hombit hombit commented Feb 7, 2019

Fix #128: find min and max values ignoring NaN

Check this GUI features before merge:

  • Positive and negative BITPIX images
  • Zoom, rotate and flip
  • Level control
  • Color maps
  • Open new image in the same window and in new window

begin, end,
std::make_pair(*begin, *begin),
Copy link
Owner

Choose a reason for hiding this comment

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

What is going to happen if begin == end? There are should not be any empty arrays, but still...

Copy link
Collaborator Author

@hombit hombit Feb 7, 2019

Choose a reason for hiding this comment

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

If FITS is empty then we have a LOT of problems in other parts of the code.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is better to consider the case of the image that is full of NaNs

Copy link
Owner

Choose a reason for hiding this comment

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

Whats about use

std::make_pair(std::numeric_limits<T>::max(), std::numeric_limist<T>::lowest())

instead of std::make_pair(*begin, *begin)?

This will lead to e containing (MAX, MIN) in both cases: when all NaNs, when iterator range is empty.
Then just handle this special case:

if (e.first > e.second)
    std::swap(e.first, e.second);

#endif
return std::make_pair(Utils::min(acc.first, x), Utils::max(acc.second, x));
Copy link
Owner

Choose a reason for hiding this comment

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

Whouldn't it better to be consistent with swap_bytes usage and ask using Utils::min; using Utils::max;?

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'd like to be as explicit as possible, a reader should understand that we use own implementation of min and max.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Let's remove using Utils:swap_bytes

@matwey
Copy link
Owner

matwey commented Feb 17, 2019

Could you please squash 2nd and 4th commits together?

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