search

MinervaExpt / MAT

Systematic uncertainty utilities for High Energy Physics experiments. Pioneered by the MINERvA experiment.
MIT License
4 stars 3 forks source link

Remove vistigial variable "is_grid" in PlotUtils::MacroUtil #6

Open smgilligan opened 2 years ago

smgilligan commented 2 years ago

After checking in the ccqenu_to_mat slack it was agreed the argument is_grid for the class PlotUtils::MacroUtil does not do anything and should be removed.

smgilligan commented 2 years ago

Completed. Please make sure to remove this argument from your own analysis code.

bamesserly commented 2 years ago

Can I ask why this was removed? Isn't this change going to break everyone's code? I like it because in my logfiles, I like to see whether the job was run on the grid or not. Maybe it's not a big deal because I can always re-add it to my child of MacroUtil implementation.

smgilligan commented 2 years ago

Sorry this caught you be surprise, Ben. For clarification I am including the discussion from Slack below.

Me:

One of the arguments passed when creating a member of PlotUtils::MacroUtil class is boolean is_grid, which I cannot find any functionality for, other than to have its value assigned to m_is_grid, which only exists to be printed for the user when MacroUtil::PrintMacroConfiguration() is executed. Am I missing something or are these now vestigial variables?

aolivier:

This is sort of a vestigial variable. I think some of the other MAT developers had a vision for what it would do in the future. I can't judge because I don't run macro jobs on the Fermigrid.

Heidi Schellman:

@Kang probably knows

Kang:

I never used that variable when I run macro jobs on the grid. I agree with Andrew, probably it is a vestigial variable.

Me:

Do we think it should be trimmed? I could make related issues in the repositories it shows up in and clear them pretty quickly.

aolivier:

Sounds good to me. Let the appendectomy begin!

This was from November 24th. I would like to say it took me over a week to do it because I was giving other people time to see the discussion and chime in, but really I just didn't get around to it because it wasn't a big priority.

bamesserly commented 2 years ago

Thanks for bringing me up to speed. Guess I missed a fun discussion by not being in the ccqenu_mat slack channel. So it's obviously not the end of the world, but just bringing up a couple more considerations:

  1. This breaks everyone's code immediately. Right now everyone passes the is_grid to their MacroUtil constructor because it didn't have a default argument. Now that it's removed, everyone's MacroUtil ctors are going to have an extra is_grid arg --> candidate constructor not viable: requires 5 arguments, but 6 were provided.
  2. Just because a variable or functionality isn't used directly in the MAT code doesn't mean it isn't used by users. Are we sure that no one's code relies on this variable?
  3. Even though my code doesn't rely on this variable, it's a useful flag to me whose value I look for inPrintMacroConfig.

Ignoring my personal stake in the variable, for reason (1) alone we might really not want to do this?

Alternatives of course are:

  1. give it a default argument in the CTOR.
  2. don't even put it in the CTOR but just have a setter and getter.
  3. at least make an announcement to the collaboration somewhere.

I'm fine with any of these 3 options.

smgilligan commented 2 years ago

You make a very fair point about the communication. I probably should have started a discussion in mat_devs and announced in advance a specific implementation time for any code breaking changes that were decided upon. Would computing be the best place for that? I think some general guidelines for implementing future changes like this might be useful, at the very least for the announcement part. I doubt everyone is paying super close attention to each repository's Issues.

Also, I don't know why the assignment of a default value didn't occur to me right away.