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

Make print(config) friendlier for copy-paste #210

Merged
merged 2 commits into from
Dec 15, 2021
Merged

Make print(config) friendlier for copy-paste #210

merged 2 commits into from
Dec 15, 2021

Conversation

mfeurer
Copy link
Contributor

@mfeurer mfeurer commented Dec 14, 2021

Implements #66.

Configurations are now printed as

Configuration(values={
  'chp': '1',
  'const': 1,
  'nfhp': 0.0,
  'nihp': 0,
  'ohp': '1',
  'ufhp': 5.5,
  'uihp': 6,
})

opposed to

Configuration:
  chp, Value: '1'
  const, Constant: 1
  nfhp, Value: 0.0
  nihp, Value: 0
  ohp, Value: '1'
  ufhp, Value: 5.5
  uihp, Value: 6

.

This does not yet allow for copy-pasting the configuration as the argument configuration_space is not printed, but it is much closer. Yet another alternative would be

{
  'chp': '1',
  'const': 1,
  'nfhp': 0.0,
  'nihp': 0,
  'ohp': '1',
  'ufhp': 5.5,
  'uihp': 6,
}

which could be used as the values argument to Configuration.

If there are any other suggestions I would be happy to hear about them.

CC @eddiebergman @KEggensperger @dengdifan @renesass @mlindauer @RaghuSpaceRajan

@RaghuSpaceRajan
Copy link
Contributor

Looks good, generally. How come you did not add the argument configuration_space? Because we do not know the variable name for the argument configuration_space?

@codecov
Copy link

codecov bot commented Dec 14, 2021

Codecov Report

Merging #210 (be027cf) into master (f71508c) will increase coverage by 1.83%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #210      +/-   ##
==========================================
+ Coverage   66.33%   68.17%   +1.83%     
==========================================
  Files          17       17              
  Lines        1613     1681      +68     
==========================================
+ Hits         1070     1146      +76     
+ Misses        543      535       -8     
Impacted Files Coverage Δ
ConfigSpace/read_and_write/pcs_new.py 92.11% <0.00%> (+4.11%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f71508c...be027cf. Read the comment docs.

@eddiebergman
Copy link
Contributor

I'm okay with either the current implementation or the pure dict one. In general, as long as print(dict(config)) gives a pure dict output, then this implementation as you have it is fine with me.

@KEggensperger
Copy link
Contributor

Yes, this is much better than before. I am fine with the current version, but I would prefer something that is actually valid code, i.e. a pure dict, over something that just looks like its valid but is then missing an argument.

@renesass
Copy link
Contributor

I like the dictionary output.

@mlindauer
Copy link
Contributor

my gut feeling is that most of the community is looking for dict-like input and output. Therefore, I would also prefer this way of printing.

@dengdifan
Copy link
Contributor

dengdifan commented Dec 15, 2021

I would prefer the current implementation. We need to be aware that the printed object is a configuration, not a dict.

@mfeurer
Copy link
Contributor Author

mfeurer commented Dec 15, 2021

Thanks everyone for your feedback. As we need to balance readability and pastability here I think the current proposal strikes a nice middle ground:

  • It keeps the information that we have a Configuration
  • by calling print(dict(config)) one gets the dictionary representation
  • it can be extended to include the ConfigSpace argument to which we don't know the variable name at print time

I just also updated the docs and enabled doctests.

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.

7 participants