Skip to content

Conversation

@lkwinta
Copy link
Contributor

@lkwinta lkwinta commented May 11, 2025

This pull request includes updates to dependencies and introduces a new scaling factor for Y-axis visualization in graph components. The changes ensure better graph rendering and maintainability of the code.

Dependency Updates:

  • Updated the jsroot dependency in package.json from version ^7.8.2 to ^7.9.0 to include the latest features and fixes.

Graph Visualization Enhancements:

  • Added a new constant MAX_SCALING_FACTOR in src/JsRoot/GraphData.tsx to define a scaling factor for the Y-axis, ensuring the maximum value is slightly larger than the actual maximum for better visualization.
  • Updated JsRootGraph1D in src/JsRoot/components/JsRootGraph1D.tsx to use MAX_SCALING_FACTOR for setting the maximum value of the Y-axis, replacing hardcoded logic.
  • Updated JsRootMultiGraph1D in src/JsRoot/components/JsRootMultiGraph1D.tsx to apply MAX_SCALING_FACTOR for consistent scaling of the Y-axis across multi-graph visualizations.

Code Refactoring:

  • Imported MAX_SCALING_FACTOR in JsRootGraph1D and JsRootMultiGraph1D components to centralize the scaling logic and improve maintainability. [1] [2]

@lkwinta lkwinta requested a review from Copilot May 11, 2025 15:27
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR upgrades the jsroot dependency and refactors histogram value assignments across components while adding a debugging console.log.

  • Updated jsroot from ^7.8.2 to ^7.9.0
  • Replaced fYaxis.fXmin/fXmax with fMinimum/fMaximum in three components
  • Added a debugging console.log in JsRootGraph1D

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

File Description
src/JsRoot/components/JsRootMultiGraph1D.tsx Refactored histogram min/max assignment
src/JsRoot/components/JsRootGraph2D.tsx Refactored histogram min/max assignment
src/JsRoot/components/JsRootGraph1D.tsx Refactored histogram min/max assignment and added console.log for debugging
package.json Upgraded jsroot dependency
Comments suppressed due to low confidence (1)

src/JsRoot/components/JsRootGraph1D.tsx:30

  • [nitpick] A console.log statement was added for debugging purposes. Consider removing or gating this debugging output for production builds.
console.log('histogram', histogram);

@lkwinta lkwinta requested review from Copilot and grzanka May 11, 2025 15:28
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR upgrades the jsroot dependency to version 7.9.0 and refactors histogram minimum and maximum assignments across multiple graph components.

  • Upgrades jsroot dependency in package.json
  • Refactors histogram value assignments from using fYaxis.fXmin/fXmax to fMinimum/fMaximum in JsRootGraph1D, JsRootGraph2D, and JsRootMultiGraph1D
  • Introduces minor adjustments to how computed maximum values are scaled by a constant factor

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

File Description
src/JsRoot/components/JsRootMultiGraph1D.tsx Updated histogram property assignments to use fMinimum and fMaximum
src/JsRoot/components/JsRootGraph2D.tsx Replaced fYaxis.fXmin/fXmax with fMinimum/fMaximum including a constant scaling factor
src/JsRoot/components/JsRootGraph1D.tsx Similar refactoring of histogram properties with a scaling factor applied to fMaximum
package.json Upgraded jsroot dependency version from ^7.8.2 to ^7.9.0

@lkwinta lkwinta requested a review from Copilot May 11, 2025 15:38
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This pull request upgrades the jsroot dependency to version 7.9.0 and refactors axis handling in the JsRoot components to use the generalized fMinimum and fMaximum properties with a configurable scaling factor. Key changes include:

  • Updating the jsroot version in package.json
  • Refactoring axis limits in JsRootGraph1D.tsx, JsRootGraph2D.tsx, and JsRootMultiGraph1D.tsx
  • Introducing MAX_SCALING_FACTOR in GraphData.tsx for consistent axis scaling

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
package.json Updated jsroot dependency version
src/JsRoot/components/JsRootGraph1D.tsx Replaced axis-specific properties with fMinimum and fMaximum
src/JsRoot/components/JsRootGraph2D.tsx Refactored axis limit handling with new fMinimum and fMaximum pattern
src/JsRoot/components/JsRootMultiGraph1D.tsx Updated multi-graph axis scaling using MAX_SCALING_FACTOR
src/JsRoot/GraphData.tsx Introduced MAX_SCALING_FACTOR constant for axis scaling consistency

@lkwinta lkwinta requested a review from Copilot May 11, 2025 15:41
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR upgrades the jsroot dependency to version 7.9.0 and improves axis handling for better visualization by replacing axis-specific properties with generic fMinimum and fMaximum values.

  • Upgraded jsroot in package.json
  • Updated one-dimensional and two-dimensional graph components to use fMinimum and fMaximum with a scaling constant
  • Introduced a MAX_SCALING_FACTOR constant in GraphData for consistent axis scaling

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
src/JsRoot/components/JsRootMultiGraph1D.tsx Replaced fYaxis properties with generic fMinimum and fMaximum using MAX_SCALING_FACTOR
src/JsRoot/components/JsRootGraph2D.tsx Updated axis limits handling to use fMinimum and fMaximum
src/JsRoot/components/JsRootGraph1D.tsx Updated axis limits handling to use fMinimum and fMaximum
src/JsRoot/GraphData.tsx Added constant MAX_SCALING_FACTOR for axis scaling
package.json Upgraded jsroot dependency to version ^7.9.0

@grzanka
Copy link
Contributor

grzanka commented May 12, 2025

Try loading "SOBP" example. The 2D map of XZ looks bad:

image

After switching to LogZ:

image

we see that the data behind is OK:

image

this is what we expect (from https://yaptide.github.io/web_dev/):

image

Copy link
Contributor

@grzanka grzanka left a comment

Choose a reason for hiding this comment

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

see comments

@grzanka grzanka requested a review from Copilot May 12, 2025 18:55
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR upgrades the jsroot dependency to version 7.9.0 and refactors the Y-axis scaling logic in graph components for improved visualization.

  • Upgraded jsroot version in package.json
  • Introduced and centralized the MAX_SCALING_FACTOR constant in GraphData.tsx
  • Updated JsRootGraph1D and JsRootMultiGraph1D to use the new scaling factor

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

File Description
src/JsRoot/components/JsRootMultiGraph1D.tsx Imports and applies MAX_SCALING_FACTOR in Y-axis scaling
src/JsRoot/components/JsRootGraph1D.tsx Applies MAX_SCALING_FACTOR for Y-axis in a single graph
src/JsRoot/GraphData.tsx Added constant MAX_SCALING_FACTOR with accompanying comment
package.json Upgraded jsroot dependency from ^7.8.2 to ^7.9.0

@lkwinta lkwinta requested review from Copilot and grzanka May 12, 2025 18:55
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR updates the jsroot dependency and refactors the graph components to use a centralized scaling factor constant for improved visualization of Y-axis data.

  • Upgraded jsroot from v7.8.2 to v7.9.0 in package.json
  • Added and applied MAX_SCALING_FACTOR in both JsRootGraph1D and JsRootMultiGraph1D for consistent Y-axis scaling
  • Centralized scaling logic by introducing MAX_SCALING_FACTOR in GraphData.tsx

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

File Description
src/JsRoot/components/JsRootMultiGraph1D.tsx Updated Y-axis scaling to use MAX_SCALING_FACTOR; replaced fYaxis properties with fMinimum/fMaximum
src/JsRoot/components/JsRootGraph1D.tsx Replaced hardcoded Y-axis calculations with use of MAX_SCALING_FACTOR for improved maintainability
src/JsRoot/GraphData.tsx Added constant MAX_SCALING_FACTOR to centralize scaling logic
package.json Updated jsroot dependency version to ^7.9.0
Comments suppressed due to low confidence (2)

src/JsRoot/components/JsRootMultiGraph1D.tsx:53

  • Ensure that replacing histogram.fYaxis.fXmin/fXmax with histogram.fMinimum/fMaximum aligns with the jsroot 7.9.0 API. If these properties have changed in behavior, consider adding a comment or reference to the documentation.
histogram.fMinimum = Math.min(...page.pages.flatMap(p => p.data.values));

src/JsRoot/components/JsRootGraph1D.tsx:26

  • Confirm that the setting of histogram.fMinimum and histogram.fMaximum (later multiplied by MAX_SCALING_FACTOR) is correct per jsroot 7.9.0. If the API now uses these properties instead of the previous fYaxis fields, a brief inline link to the update notes would be helpful.
histogram.fMinimum = Math.min(...y);

Copy link
Contributor

@grzanka grzanka left a comment

Choose a reason for hiding this comment

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

LGTM

@grzanka grzanka added this pull request to the merge queue May 12, 2025
Merged via the queue into master with commit 8a7d144 May 12, 2025
10 checks passed
@grzanka grzanka deleted the bump_jsroot branch May 12, 2025 19:15
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.

3 participants