Skip to content

refactor!: massive cleanup#293

Merged
c-dilks merged 56 commits intomainfrom
v3
Apr 16, 2025
Merged

refactor!: massive cleanup#293
c-dilks merged 56 commits intomainfrom
v3

Conversation

@c-dilks
Copy link
Copy Markdown
Member

@c-dilks c-dilks commented Mar 7, 2025

This is a very large PR, I got carried away. The changes and reasoning are documented in the following description, organized by type of change:

Bug Fixes

Although this PR is supposed to be a refactor, the following bugs were fixed:

  • timelines RF -> "rftime 12 diff*" are now being filled with data; the RF period from CCDB was not actually being used in monitor2p2GeV.java
  • reliance on external coatjava and having a groovy version that matches the POM version are no longer needed; the Maven build is now effectively standalone

Renames

  • "monitoring" Java classes are now referred to as "histogramming" classes, and
    are all part of the package org.jlab.clas.timeline.histograms
    • they are now run by run_histograms.java (renamed from ana_2p2)
  • "detectors" Groovy classes, for detector timelines, are now referred to as
    "analysis" classes, and are all part of the package org.jlab.clas.timeline.analysis
    • they are now run by run_analysis.groovy (renamed from run_detectors.groovy)
  • updated documentation accordingly
  • renamed the following histogramming classes:
    • cndCheckPlots.java -> CND.java
    • central.java -> CTOF.java
    • monitor2p2GeV.java -> GeneralMon.java
    • tof_monitor.java -> DCandFTOF.java
  • rename analysis class methods for consistency with histogramming class methods:
    • processDirectory -> processRun (cf. histogramming classes' processEvent)
    • close -> write (cf. histogramming classes' write)

Removals

Note

Anything we have removed here can be recovered from git history, if someone needs it

Histogramming Class Cleanup

  • remove unused variables and includes
    • variables that are obviously unused have been deleted
    • variables that may have been used for debugging have been commented out
  • remove retrieval of unused banks in histogramming's processEvent methods
    • NOTE: this should hopefully speed things up a bit
  • run_histograms.java (renamed from ana_2p2.java) should only have to call
    processEvent and write for each histogramming class
    • this makes calling the monitor classes a bit more general: for each event,
      call processEvent, at the end, call write
    • in some cases, two methods were called after event processing; these
      additional methods are now just called by write instead
    • there are still variations in the methods' parameters, as well as their
      constructors, but that's fine for now
  • remove main and plot methods
    • they just mostly contained stuff to make plot PNGs
    • plot viewing should use the web front-end, not custom canvases and PNG
      output; we can allow webserver access to those who need it.
  • remove any other unused methods

Histogramming Class Removals

We have removed the following histogramming classes, none of which have been
used in quite a while:

  • occupancies.java, for BST and BMT occupancy
    • BST and BMT occupancies appear to be handled by monitor2p2
  • deuterontarget.java
    • many plots are handled elsewhere, namely by monitor2p2
  • dst_mon.java
    • The produced histograms are not being plotted; they have been disabled for
      some time, since Daniel Carman said they are not needed.
    • There are several other histograms being filled in dst_mon, beyond the
      FTOF ones; however, most of them are either unused or produced by something
      else, namely monitor2p2.

Analysis Class Removals

  • remove unused CTOF and FTOF analysis groovy classes
    • these all have been disabled for some time, since Daniel Carman said they
      are not needed.

Build Changes (Maven)

  • Use the same compiler plugin for all modules: groovy-eclipse-compiler
    • we are already using it for the Groovy code
    • it can handle mixed Java+Groovy projects pretty well
    • required adding a new Maven repo, so we can use the latest version
      • told dependabot to track it too
  • combine detectors and monitoring modules to a single module
  • organize the source tree; we now have the following packages:
    • org.jlab.clas.timeline: main package for all timeline code
    • org.jlab.clas.timeline.histograms: reads HIPO data files and makes histograms
    • org.jlab.clas.timeline.analysis: analyzes histograms
    • org.jlab.clas.timeline.fitter: fits histograms; analysis classes call fitter classes
    • org.jlab.clas.timeline.util: miscellaneous common stuff
  • do not require groovy JARs to be in caller classpath
    • The current kluge to find Groovy JAR files at runtime fails in Arch
      Linux (and therefore in our container images), but has been working fine
      in production on ifarm. We should not need to add Groovy JARs to the
      class path; instead we can simply copy dependencies to the installation
      directory. I added the script install.sh, since the mvn command is
      slightly complicated.
    • now we no longer have to have coatjava's JAR file and Groovy JAR files
      in the class path (in environ.sh); we just need target and
      target/dependency.
      • we have also removed any need for coatjava environment variables ($COATJAVA, $PATH, etc.)
    • strangely, this required us to switch from YamlSlurper to snakeyaml, because
      of some dependency conflict
  • force all groovy calls to use the Maven build's version of groovy
    • to make sure there are no version conflicts with a user's system groovy
    • replaces Coatjava's run-groovy with our run-groovy-timelines.sh
    • cleaned up associated environment variables etc.
  • correct the version number in pom.xml
    • we'll call this 2.99.0, since we have more work to do before tagging 3.0.0

Miscellaneous

  • make sure the URI for RCDB is consistent everywhere, using environment variable $RCDB_CONNECTION

Style Changes

  • fix indentation in Java files; replace tabs with spaces
  • resisted the urge to run an auto-formatter, to avoid making the git diff impossible to understand

c-dilks added 27 commits March 7, 2025 15:03
they do not appear to be used; we can recover them from `git` history if
someone is relying on them (but they should be encouraged to use the
more general, in-use code)
This fixes all the current compilation warnings.

IMPORTANT: many `processeEvent` methods were retrieving _several_ more
banks than the method actually needed; this commit removes several
unnecessary `DataEvent.getBank` calls.

In general:
- variables that are obviously unused have been deleted
- variables that may have been used for debugging have been commented
  out
Plot viewing should use the web front-end, not custom canvases and PNG
output; we can allow webserver access to those who need it.

Similar to the removal of `main` methods, if someone depends on `plot`
methods, we may restore them from `git` history. We remove unused code
so that we don't have to waste time maintaining it.
…ite`

This makes calling the monitor classes a bit more general:
- for each event, call `processEvent`
- at the end, call `write`

In some cases, two methods were called after event processing;
additional methods are now just called by `write`.

There are still variations in the methods' parameters, as well as their
constructors.
All source code for detectors timelines is now in `src/` and compiles as
a single module. We now have the following packages:
- `org.jlab.clas.timeline`: main package for all timeline code
- `org.jlab.clas.timeline.histograms`: reads HIPO data files and makes
  histograms
- `org.jlab.clas.timeline.analysis`: analyzes histograms
- `org.jlab.clas.timeline.fitter`: fits histograms; `analysis` classes
  call `fitter` classes
- `org.jlab.clas.timeline.util`: miscellaneous common stuff
The current method to find Groovy JAR files at runtime fails in Arch
Linux (and therefore in our container images), but has been working fine
in production on `ifarm`. We should not need to add Groovy JARs to the
class path; instead we can simply copy dependencies to the installation
directory. I added the script `install.sh`, since the `mvn` command is
slightly complicated.

Now we no longer have to have `coatjava`'s JAR file and Groovy JAR files
in the class path (in `environ.sh`); we just need `target` and
`target/dependency`.
```
ana_2p2.java         -> run_histograms.java
run_detectors.groovy -> run_analysis.groovy
```
Seems that Apache has been wishy-washy regarding which modules are optionally
included in `groovy-all` or not...
Using `coatjava`'s `run-groovy` will pull in that `coatjava` build's
`CLASSPATH`, which may have conflicts with the `coatjava` JARs we use
here in the timeline build.

Since we set our own `CLASSPATH` here, use `groovy` as is, instead of
`run-groovy`, for better control over the class path.
Using the maven-packaged `groovy` means there won't be a conflict
between the user's system `groovy` version and the JAR files used by the
`maven` build.

Switching from `YamlSlurper` to `snakeyaml` should hopefully finally
resolve the `snakeyaml` version conflict issue.
Now that we have our own `groovy` wrapper, `run-groovy-timeline.sh`, we
can use it to set preferred `groovy` options.
- has not been used for quite a while
- BST and BMT occupancies appear to be handled by `monitor2p2`
- has not been used in a while
- many plots are handled elsewhere, namely by `monitor2p2`
```
cndCheckPlots.java -> CND.java
central.java       -> CTOF.java
tof_monitor.java   -> FTOFandDC.java
```
Comment thread .github/dependabot.yml
c-dilks added a commit to JeffersonLab/clas12-env that referenced this pull request Mar 27, 2025
JeffersonLab/clas12-timeline#293 removes the
explicit need for `groovy` and `coatjava` as external dependencies; the
Maven build will just copy the necessary JAR files for these, so now the
timeline code is much more standalone. RCDB remains as an external
dependency. See the PR for reasoning and details.

This should not be merged until we have actually deployed v3.
c-dilks added 6 commits March 27, 2025 13:04
src/main/java/org/jlab/clas/timeline/histograms/FTOFandDC.java -> src/main/java/org/jlab/clas/timeline/histograms/DCandFTOF.java
src/main/java/org/jlab/clas/timeline/histograms/monitor2p2GeV.java -> src/main/java/org/jlab/clas/timeline/histograms/GeneralMon.java
Comment thread pom.xml
@c-dilks c-dilks added this to the v3 milestone Mar 30, 2025
c-dilks added a commit to JeffersonLab/clas12-env that referenced this pull request Apr 16, 2025
JeffersonLab/clas12-timeline#293 removes the
explicit need for `groovy` and `coatjava` as external dependencies; the
Maven build will just copy the necessary JAR files for these, so now the
timeline code is much more standalone. RCDB remains as an external
dependency. See the PR for reasoning and details.

This should not be merged until we have actually deployed v3.
c-dilks added a commit to JeffersonLab/clas12-env that referenced this pull request Apr 16, 2025
JeffersonLab/clas12-timeline#293 removes the
explicit need for `groovy` and `coatjava` as external dependencies; the
Maven build will just copy the necessary JAR files for these, so now the
timeline code is much more standalone. RCDB remains as an external
dependency. See the PR for reasoning and details.

This should not be merged until we have actually deployed v3.
@c-dilks c-dilks merged commit 7ff235b into main Apr 16, 2025
10 checks passed
@c-dilks c-dilks deleted the v3 branch April 16, 2025 21:04
@Sangbaek
Copy link
Copy Markdown
Collaborator

@c-dilks ,This is amazing. I appreciate your work.
But just to make sure, is the offline monitoring (not timeline but a lot of stamp sized plots for one run) a legacy thing, which no one tries to look into?
When I worked on the timeline, there were a number of people, who wanted to keep both.
I prefer to keep the timeline code only, because now that's what the CALCOM focuses on.
Btw, I am going to include alert in the directory, so please stay tuned.

@Sangbaek
Copy link
Copy Markdown
Collaborator

I also like you throw away the 2p2. No one told me the truth, but I am really sure that it's from FX's naming about analyzing 2.2 GeV commissioning data. It doesn't make much sense to keep 2p2 in 2025.

@c-dilks
Copy link
Copy Markdown
Member Author

c-dilks commented Apr 17, 2025

Hi @Sangbaek,

But just to make sure, is the offline monitoring (not timeline but a lot of stamp sized plots for one run) a legacy thing, which no one tries to look into?

I assumed it was legacy; it hasn't been used by chefs or the CI tests and therefore it got removed, but we can recover it if needed. @dcarman, @raffaelladevita, @baltzell, @drewkenjo may know more about the history here (tagging them in case they want to comment).

Btw, I am going to include alert in the directory, so please stay tuned.

Sounds good! Now that this massive PR is merged, it's a good time to get started on that.

I also like you throw away the 2p2. No one told me the truth, but I am really sure that it's from FX's naming about analyzing 2.2 GeV commissioning data. It doesn't make much sense to keep 2p2 in 2025.

Ah, I've been wondering about that name. Hopefully the new names are clearer. I couldn't think of a better name than GeneralMon (formerly monitor2p2GeV); better ideas are welcome!

baltzell pushed a commit to JeffersonLab/clas12-env that referenced this pull request Apr 21, 2025
JeffersonLab/clas12-timeline#293 removes the
explicit need for `groovy` and `coatjava` as external dependencies; the
Maven build will just copy the necessary JAR files for these, so now the
timeline code is much more standalone. RCDB remains as an external
dependency. See the PR for reasoning and details.

This should not be merged until we have actually deployed v3.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants