consistency with IBPSA

[Mathadon]

@dhblum One thing that I lost track of: if we now copy models from IBPSA into IbpsaMpc manually, we will gradually diverge from IBPSA since new changes of IBPSA don't get merged into IbpsaMpc. To avoid this, it would make sense to set up some way to automate the merging process. Two problems here:

1) The buildingspy merger could be used for the merging, but it's not practical to use it for merging only a limited set of models (while it's easy to exclude a limit set of models). This functionality should thus be added to buildingspy. @mwetter do you agree? 2) The merging will overwrite our custom changes to the models that make them compatible with JModelica. One approach to reapply the changes is to use git cherry-pick, although I'm not sure whether this will work as intended, especially when merge conflicts arise. We would also have to keep track of all commits that modify IBPSA code.

Perhaps there is a better way but we better look into this before making too many PRs?

[mwetter]

For the merger script, we could have a list of models to be merged to the IbpsaMpc repository, but as you wrote, this would overwrite custom changes done to the file in the IbpsaMpc repository.

On Wed, Aug 14, 2019 at 10:34 AM Filip Jorissen notifications@github.com wrote:

@dhblum https://github.com/dhblum One thing that I lost track of: if we now copy models from IBPSA into IbpsaMpc manually, we will gradually diverge from IBPSA since new changes of IBPSA don't get merged into IbpsaMpc. To avoid this, it would make sense to set up some way to automate the merging process. Two problems here:

1. The buildingspy merger could be used for the merging, but it's not practical to use it for merging only a limited set of models (while it's easy to exclude a limit set of models). This functionality should thus be added to buildingspy. @mwetter https://github.com/mwetter do you agree?
2. The merging will overwrite our custom changes to the models that make them compatible with JModelica. One approach to reapply the changes is to use git cherry-pick, although I'm not sure whether this will work as intended, especially when merge conflicts arise. We would also have to keep track of all commits that modify IBPSA code.

Perhaps there is a better way but we better look into this before making too many PRs?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ibpsa/modelica-ibpsa-mpc/issues/4?email_source=notifications&email_token=AAKEPOEAJP4O3IMGOYFAGCDQEQ6LRA5CNFSM4ILXG63KYY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4HFINRMA, or mute the thread https://github.com/notifications/unsubscribe-auth/AAKEPOEMIKXDTCH2ZEIBWC3QEQ6LRANCNFSM4ILXG63A .

[Mathadon]

For the merger script, we could have a list of models to be merged to the IbpsaMpc repository, but as you wrote, this would overwrite custom changes done to the file in the IbpsaMpc repository. …

Just to confirm, that functionality does not exist yet in Buildingspy, right?

[mwetter]

Yes, you would need to add this functionality. If I recall correctly, if a file exists in the target library and is listed in copiedFiles.txt, it will be merged. If it exists in the target library but is not listed in copiedFiles.txt, it will not be merged.

[dhblum]

I think we discussed before that there will be three types of models in the library, with reference to the IBPSA library:

1) Models that can be copied directly from IBPSA 2) Models that can be copied directly from IBPSA but have default parameter values changed so as to allow for use in MPC (e.g. reverseFlow = False) 3) Models that are based on IBPSA models but have syntax or other custom changes so as to allow for use in MPC.

For 1., is solved by a merge script using buildingspy with a custom include list, as you've suggested.

For 2., I think could be solved by having the user-facing mpc library models extend base ibpsa models with edited parameters.

For 3., I'm not sure of a good way to automatically merge. The options perhaps are to:

• i. See if the syntax can be changed also in the base ibpsa library without consequence, and if so, make a pull request to that library to make the change.

• ii. Maybe have a script that checks which model files are changed in an ibpsa update that correspond to mpc library models that are based on those models with custom code modifications, and returns the names of affected files. Then at least we can see which models to look at manually and perform the necessary edits? Just a brainstorm here.

[Mathadon]

For 1) and 2) we could write a new merger functionality that takes a .json as input that includes the parameters that should be overwritten. If the parameter set is empty then we simply copy the file, otherwise we duplicate it and extend it with the required parameters. I think it's best to start a new merger function for this as integrating it with the existing code may become a mess.

For 3) I would wait to automate this but one way to go would be to keep track of the changes we performed and add the respective commits to a cherry-pick list that is run after each merge..

[dhblum]

For 1) and 2) you mean integrating with existing buildingspy code?

I don't see why the use of cherry-pick wouldn't work for 3). But as you mentioned before, we have to be careful about tracking which commits should be applied (and what is contained in those commits). We could create a specific issue label to help us with tracking these changes. Kind of like a "non-backwards-compatible" label. Maybe "modified-ibpsa-code"?

[Mathadon]

1) and 2) yes

3) Can't a cherry-pick be used to re-apply a commit onto code? That would allow us to re-apply changes.

[dhblum]

Yes, I think it should work (maybe I shouldn't have used a double negative in my earlier comment?). I'm just suggesting that we can flag issues that require this to help keep track of which commits should be on/added to the cherry-pick list.

[Mathadon]

I guess that most issues would need the flag. As part of the merge process we could require the developer to append the relevant sha's to a predefined txt file in the repository?

[dhblum]

Yes, that would make sense. As part of the review process, the reviewer should also make sure the listed sha's only commit the relevant changes, otherwise we could end up with unexpected conflicts over time.

[Mathadon]

@dhblum see https://github.com/Mathadon/BuildingsPy/tree/issue278_mergeMpc for a first working version of the merge script.

E.g. use

import buildingspy.development.merger as m
import os
modelicapath = os.environ["MODELICAPATH"]
ibpsa_dir = os.path.join(modelicapath, "IBPSA")
dest_dir = os.path.join(modelicapath, "IbpsaMpc")
mer = m.IBPSA(ibpsa_dir, dest_dir) # doctest: +SKIP

files={"IBPSA.Fluid.Sources.Boundary_pT": {},
"IBPSA.Fluid.Sources.Boundary_ph": {"use_h_in" : "true"},
        }

mer.mergeIbpsaMpc(files)

This only implements 'type 1' and 'type 2' merges.

[Mathadon]

3 uses this functionality but does not yet override parameters. For the movers/sensors we will however need it.

[dhblum]

This issue is addressed by https://github.com/ibpsa/modelica-ibpsa-mpc/issues/8. Can it be closed?