Skip to content

feat: implement modified stage memory reset and scalar plotting for tensors - #9

Open
mattia-spider wants to merge 1 commit into
camml-lab:developfrom
mattia-spider:feature/generalised-nmr-tensors-plotter
Open

feat: implement modified stage memory reset and scalar plotting for tensors#9
mattia-spider wants to merge 1 commit into
camml-lab:developfrom
mattia-spider:feature/generalised-nmr-tensors-plotter

Conversation

@mattia-spider

Copy link
Copy Markdown
Collaborator
  • Moved data cleaning at the beginning of stages
  • Added _plot_combined_all_stages function to get parity plot with all stages combined at end on testing if requested.
  • Added TensorsGraphParityPlotter to compute and plot requested properties of tensors, building up on ParityPlotter and GraphParityPlotter logic.
  • Generalized _plot_parity e _plot_combined_all_stages in TensorsGraphParityPlotter to plot dictionaries with multiple keys.

@mattia-spider
mattia-spider requested a review from muhrin April 28, 2026 14:41
@codecov-commenter

codecov-commenter commented Apr 28, 2026

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 21.42857% with 88 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/tensorial/reaxkit/listeners/parity_plotter.py 21.42% 88 Missing ⚠️
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.
Files with missing lines Coverage Δ
src/tensorial/reaxkit/listeners/parity_plotter.py 57.69% <21.42%> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@muhrin muhrin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this needs some thinking. I would start with generalising out the plotting of individual plots onto a single matplotlib.Figure and then go from there, I think you will find that the code gets a lot shorter and a lot simpler.

filename = f"{stage_name}_epoch_{epoch}.pdf" if epoch is not None else f"{stage_name}.pdf"
plt.savefig(str(save_dir / filename), bbox_inches="tight")
# full_path = save_dir / filename
# print(f"DEBUG: Saving plot to {full_path.absolute()}")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This you can remove


# 2. Clear the stage data for the next run (e.g., next 'fit' call)
self.data_store[stage_name] = ([], [])
# self.data_store[stage_name] = ([], [])

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Remove this


y_true, y_pred = self.get_target_predicted(batch, outputs)

# _LOGGER.info(f"Collected {len(y_true)} points for {stage_name}") # Log temporaneo

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This can be removed, I think?

}
self._last_plotted_epoch.clear()

def _plot_combined_all_stages(self, trainer: "reax.Trainer", key_name: str | None = None):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This function violates the DRY principle (https://en.wikipedia.org/wiki/Don't_repeat_yourself). If you look above, you have code that does pretty much the same thing in _plot_parity and here you replicate it to plot multiple datasets on the same plot. Instead, you should have a single function that adds a lot to an existing figure (taking a fig object and anything else it needs), and then when you want to plot a 'combined' plot, you just loop over calling that and finally call savefig().

"""
def __init__(
self,
targets: str = "nodes.nmr_tensors",

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This cannot be hardcoded to NMR tensors. Remember, this is a general class that should not even know what an NMR tensor is.

**kwargs
)

self.scalar_keys = scalar_keys or [

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

As we discussed, this is problematic because you've hard coded it to rank-2 tensors...ideally it would just plot a series of irreps.

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