Skip to content

Release Output module feature - #420

Open
adrianarce-elemwave wants to merge 145 commits into
devfrom
feat/output-module-production-readiness
Open

Release Output module feature#420
adrianarce-elemwave wants to merge 145 commits into
devfrom
feat/output-module-production-readiness

Conversation

@adrianarce-elemwave

Copy link
Copy Markdown
Collaborator

No description provided.

@adrianarce-elemwave
adrianarce-elemwave force-pushed the feat/output-module-production-readiness branch 3 times, most recently from 40f87ba to 60ba189 Compare September 2, 2026 12:00
@adrianarce-elemwave
adrianarce-elemwave force-pushed the feat/output-module-production-readiness branch from 60ba189 to 1456bee Compare September 2, 2026 13:04

@lmdiazangulo lmdiazangulo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think there is an issue with the sampling period being ignored for movies. Could you review that? We should add some specific tests to ensure this works properly.

My conversation with copilot:

User: This branch is changing the way outputs are provided. I think that samplingPeriod is being ignored for movies. Is it true?

GitHub Copilot: I’ll trace samplingPeriod from input parsing through movie scheduling and compare it with the nearby output implementation. The key check is whether movie writes are gated by that value or only by the global timestep loop.

The parser and movie module both contain the concept, so the likely control point is the new update_movie_probe_output path rather than JSON parsing. I’m narrowing in on the parsed domain fields, the output type’s timing state, and the exact write guard now; that will distinguish “parsed but unused” from “applied under a different name.”

The evidence is now fairly direct: readVolProbe copies the JSON sampling period into VolProbe_t%tstep, and preprocessing copies it into the observation schedule. But the new movie updater increments nTime and saves on every solver call, with no visible check against this%domain%tstep; I’m checking preprocessing and the save helpers once to confirm whether scheduling is supposed to happen upstream.

Yes. In this branch, samplingPeriod is parsed and copied into the movie’s domain%tstep, but it is not used when sampling.

  • JSON parsing stores it in VolProbe_t%tstep in smbjson.F90:1793-1795.
  • Output preprocessing carries it into domain%tstep in output.F90:550-565.
  • However, update_movie_probe_output increments nTime and saves data on every solver timestep in movieProbeOutput.F90:256-272, without checking domain%tstep, tstart, or tstop.

The movie save helpers then record the current solver time directly, for example movieProbeOutput.F90:682-687.

So a movie currently samples at the simulation timestep, effectively ignoring samplingPeriod. The existing movie test does not catch this because it only runs one timestep.

@lmdiazangulo lmdiazangulo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The ngspice submodule should point to the one in the dev branch. I believe there is no need to change it in with this PR.

@adrianarce-elemwave
adrianarce-elemwave force-pushed the feat/output-module-production-readiness branch from 2951efb to 6d68de0 Compare September 8, 2026 07:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants