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

Size cleanup #371

Merged
merged 9 commits into from
Oct 9, 2024
Merged

Size cleanup #371

merged 9 commits into from
Oct 9, 2024

Conversation

ceriottm
Copy link
Contributor

@ceriottm ceriottm commented Oct 8, 2024

Fixes #369. I cleaned up a bit the point-size-scaling machinery in a way that hopefully addresses that problem, and makes it more robust and self-explanatory.

Luthaf and others added 5 commits October 7, 2024 14:15
This PR addresses some issues around the "size" scaling that was
not very usable beside the basic usage. Several changes:
* Scaling is *area based* (which is the perceptually right thing)
* Removed the "reverse" button and switch that was actually meaningless
  in many cases
* Differentiate between linear and flipped-linear mapping (that span the
  whole min-max range) and "absolute" mappings including proportional,
  log, sqrt, inverse that instead use an "absolute" scale

NB this is another breaking change as it removes the "reverse"
option that was frankly useless
@ceriottm ceriottm requested a review from Luthaf October 8, 2024 14:28
This PR addresses some issues around the "size" scaling that was
not very usable beside the basic usage. Several changes:
* Scaling is *area based* (which is the perceptually right thing)
* Removed the "reverse" button and switch that was actually meaningless
  in many cases
* Differentiate between linear and flipped-linear mapping (that span the
  whole min-max range) and "absolute" mappings including proportional,
  log, sqrt, inverse that instead use an "absolute" scale

NB this is another breaking change as it removes the "reverse"
option that was frankly useless
Copy link
Contributor

@Luthaf Luthaf left a comment

Choose a reason for hiding this comment

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

I'm not sure I like this. When there are negative values in the dataset, this will look like there are missing points.

Why/where is the previous code not working?

<option value="linear" title="Linear mapping over the full range of values (large value -> large size)">linear</option>
<option value="flip-linear" title="Linear reverse mapping over the full range of values (large value -> small size)">flipped linear</option>
<option value="proportional" title="Proportional to actual value, breaks for negative values">proportional</option>
<option value="log" title="Log-scale of actual value, breaks for negative values">log</option>
Copy link
Contributor

Choose a reason for hiding this comment

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

The title does not show for me anywhere. Can you see it in the rendered HTML?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the title should (and does, for me) show up as a tooltip if you hover the mouse over the option.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, right! It just take a second for it to appear

@ceriottm ceriottm mentioned this pull request Oct 8, 2024
@ceriottm
Copy link
Contributor Author

ceriottm commented Oct 8, 2024

I'm not sure I like this. When there are negative values in the dataset, this will look like there are missing points.

Why/where is the previous code not working?

For instance, the log would have negative values, that would render as the absolute value so small values give larger points. Also in general, given we don't have a "scale" to refer to, it is really hard (should I say impossible?) to interpret a sqrt(x), log(x) or 1/x scaling if it is referred to a shifted range.

@Luthaf
Copy link
Contributor

Luthaf commented Oct 8, 2024

I guess that's fair, but then I would do like when using a log scale for the x/y/z axis and (a) send a warning to the user instead of doing this silently, and (b) ideally drop/hide the corresponding points

@ceriottm
Copy link
Contributor Author

ceriottm commented Oct 8, 2024

I guess that's fair, but then I would do like when using a log scale for the x/y/z axis and (a) send a warning to the user instead of doing this silently, and (b) ideally drop/hide the corresponding points

Fair. Done ^_^

@ceriottm ceriottm requested a review from Luthaf October 9, 2024 08:43
@Luthaf Luthaf merged commit a69d8b9 into main Oct 9, 2024
6 checks passed
@Luthaf Luthaf deleted the size-cleanup branch October 9, 2024 09:01
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.

chemiscope local web app size 0
2 participants