Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add diff_from(self, other) method to CfgNode #53

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Zeltserj
Copy link

@Zeltserj Zeltserj commented Apr 13, 2022

I needed this in my work and thought it would be cool to incorporate:

Given 2 CfgNodes:

Cfg1:

SYSTEM:
  NUM_GPUS: 2
TRAIN:
  SCALES: (1, 2)
DATASETS:
  train_2017:
    17: 1
    18: 1

Cfg2:

SYSTEM:
  NUM_GPUS: 2
TRAIN:
  SCALES: (4, 5, 8)
DATASETS:
  train_2017:
    17: 1
    18: 1

calling cfg1.diff_from(cfg2) returns another CfgNode instance, which is the following:

add:
  
change:
  TRAIN:
    SCALES: (8, 16, 32)
minus:
  

@luowyang
Copy link

luowyang commented Apr 13, 2022

Cool idea, but how are the following cases handled:

  1. cfg1 has additional key/value mapping than cfg2
  2. cfg1 has missing key/value mapping that is present in cfg2

@Zeltserj
Copy link
Author

Thanks for responding quickly.

  1. they won't be in the new config
  2. they will be in the new config.

While this suits my use case (only changes in values, no additional keys), it is not consistent, so I'll change it.
I can either:

  1. Throw an error that diff_from requires same keys
  2. add all additional keys to the output

WDYT?

@luowyang
Copy link

luowyang commented Apr 13, 2022

I believe the intuitive semantics for cfg1.diff_from(cfg2) is cfg1 - cfg2 , meaning "how cfg1 differs from cfg2". In other words, the method should give us the change from cfg2 to cfg1.

Personally I recommend the solution that the method produces a new CfgNode which contains tree sub-nodes, add, minus and change. It is up to the users to decide what they want to do with each sub-node. Such behavior is more consistent with git diff.

Besides, it should be made clear whether the key/value mappings in the new CfgNode are shallow or deep copies of the original ones. Those mappings in cfg1 and cfg2 may change after diff_from returns.

@Zeltserj
Copy link
Author

I agree. Good idea. Changed accordingly

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.

2 participants