#301 List of available config accessible for end-users by Didou09 · Pull Request #327 · ToFuProject/tofu

Didou09 · 2020-01-13T11:13:36Z

Motivations:

As pointed out by issue #301 , in the online tutorial for plotting the gallery of fusion machines, we ask the user to read a hidden variable tf.geom.utils._DCONFIG_TABLE.

This is not good practice, a public method should exist to list all available pre-implemenetd configurations.

Main changes:

Two new methods are created, a hidden and a public:

tf.geom.utils._get_listconfig() : a hidden method used to generate the dict of (unique keys, shortcuts) from hidden variables _DCONFIG and _DCONFIG_TABLE and return either a fully formatted str message or the dict itself. It is used to provide the core message of the public method, and also re-used to properly generate the error message when tf.geom.utils.create_Config() is not properly called by the user.
tf.geom.utils.get_available_config(): a public method that prints a fully formatted explicit message containing all available keys to pre-implemented config

Example:

In [1]: import tofu as tf

In [2]: tf.geom.utils.get_available_config()
A config is the geometry of a tokamak
You can define your own, see online tutorial at:
    https://tofuproject.github.io/tofu/auto_examples/tutorials/tuto_plot_create_geometry.html
tofu also also provides some pre-defined config ready to load
They are available via their name or via shortcuts

	unique names	shortcuts
	------------ 	-------------------------
	- ITER-V1     	['ITER-V1', 'A2']
	- ITER-V2     	['ITER-V2', 'B4', 'ITER']
	- JET-V0      	['JET-V0', 'JET']
	- NSTX-V0     	['NSTX-V0', 'NSTX']
	- WEST-Sep    	['WEST-Sep', 'A3']
	- WEST-V1     	['WEST-V1', 'A1']
	- WEST-V2     	['WEST-V2', 'B1']
	- WEST-V3     	['WEST-V3', 'B2']
	- WEST-V4     	['WEST-V4', 'B3', 'WEST']

  => to get a pre-defined config, call for example:
	config = tf.geom.utils.create_config('ITER')

Issues:

Fixes, in devel, issue #301

…e explicit msg

pep8speaks · 2020-01-13T11:13:41Z

Hello @Didou09! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-01-17 15:45:54 UTC

Didou09 · 2020-01-13T11:18:48Z

-# Default config
-_DEFCONFIG = 'ITER'
+
+def _get_listconfig(dconfig=_DCONFIG, dconfig_table=_DCONFIG_TABLE,


core hidden method
It is the base that provides either a dict or a str (formatted message) containing the core info

This is great. However, I would suggest adding a short docstring to the new method and its intended use.
In particular, this would allow to describe in words what _DCONFIG and _DCONFIG_TABLE is
(to me, it's not clear what the difference is).

Didou09 · 2020-01-13T11:19:09Z

+        return msg
+
+
+def get_available_config(dconfig=_DCONFIG, dconfig_table=_DCONFIG_TABLE,


Public method for end-users

Same here, a docstring would be nice.

Also, since this is a new public method, it should be added to __all__ no? As far as I can tell this is not the case.

Otherwise the message is nicely written, especially the use of an URL is good I believe.

Done, thanks :-)

Didou09 · 2020-01-13T11:19:31Z

-               + "\t - unique keys: {}\n".format(list(dconfig.keys()))
-               + "\t - shortcuts  : {}\n\n".format(list(dconfig_table.keys()))
-               + "  => you provided: case = {}\n".format(config))
+        msg = ("\nThe provided config name is not valid.\n"


This error message re-uses the formatted message created by the hidden method

… tf.geom.utils.create_Config()

codecov-io · 2020-01-14T17:18:36Z

Codecov Report

❗ No coverage uploaded for pull request base (devel@f7444b9). Click here to learn what that means.
The diff coverage is 0.97%.

@@           Coverage Diff            @@
##             devel     #327   +/-   ##
========================================
  Coverage         ?   40.04%           
========================================
  Files            ?       79           
  Lines            ?    24111           
  Branches         ?        0           
========================================
  Hits             ?     9656           
  Misses           ?    14455           
  Partials         ?        0

Impacted Files	Coverage Δ
tofu/geom/_core.py	`63.19% <0%> (ø)`
tofu/data/_plot.py	`91.2% <0%> (ø)`
tofu/imas2tofu/_core.py	`0.67% <0%> (ø)`
tofu/utils.py	`47.93% <0%> (ø)`
tofu/data/_core.py	`38.97% <0%> (ø)`
tofu/data/_comp.py	`19.93% <0%> (ø)`
tofu/__init__.py	`93.93% <100%> (ø)`
tofu/version.py	`100% <100%> (ø)`
tofu/geom/utils.py	`42.39% <100%> (ø)`

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 f7444b9...7340194. Read the comment docs.

flothesof

Good work :)
I've spotted one major thing (all missing) and some minor stuff (docstrings).

flothesof · 2020-01-17T13:20:14Z

-# Default config
-_DEFCONFIG = 'ITER'
+
+def _get_listconfig(dconfig=_DCONFIG, dconfig_table=_DCONFIG_TABLE,


This is great. However, I would suggest adding a short docstring to the new method and its intended use.
In particular, this would allow to describe in words what _DCONFIG and _DCONFIG_TABLE is
(to me, it's not clear what the difference is).

flothesof · 2020-01-17T13:22:36Z

+        return msg
+
+
+def get_available_config(dconfig=_DCONFIG, dconfig_table=_DCONFIG_TABLE,


Same here, a docstring would be nice.

Also, since this is a new public method, it should be added to __all__ no? As far as I can tell this is not the case.

Otherwise the message is nicely written, especially the use of an URL is good I believe.

…cluded new method in tf.geom.utils.__all__

…ect/tofu into Issue301_GetListConfigs

flothesof

One more small issue.

Other than that docstrings look great and thanks for the renaming of vars, I think it improves readability.

flothesof · 2020-01-17T15:32:47Z

+
+def get_available_config(dconfig=_DCONFIG,
+                         dconfig_shortcuts=_DCONFIG_SHORTCUTS,
+                         verb=True, returnas=False):


Why is returnas a bool here? Shouldn't it be str?
Below there's a check if returnas is str...
What do you think?

Nope, in general in many tofu functions / methods, returnas can be:

False => nothing is returned

None / True => set to default return format

a specific format (here str)

It is because in this function there is only one output possible format, but that may evolve in the future.
The use of True / None is to specify we want output but either the user doesn't know that he has choice between several formats (in that case it is usually set to the most intuitive) or to let the format be decided later by another function.

I agree in this case we just have a choice between False (default) and str, but this way we keep the general workflow / convention

I did not implement True / None because I forgot though,
But in 99 % cases users will not ask for returning anything

…config() (default to str)

[Issue301] Implemented tf.geom.utils.get_available_config(), with mor…

6503b99

…e explicit msg

Didou09 added the geom - improve label Jan 13, 2020

Didou09 requested review from flothesof and lasofivec January 13, 2020 11:13

Didou09 self-assigned this Jan 13, 2020

[Issue301] PEP8 compliance 1

f2f9cb4

Didou09 commented Jan 13, 2020

View reviewed changes

Didou09 changed the title ~~List of available config accessible for end-users~~ #301 List of available config accessible for end-users Jan 13, 2020

VEZINET Didier added 2 commits January 14, 2020 18:06

[Issue301] Error message now used also when user provides no input to…

995809f

… tf.geom.utils.create_Config()

[Issue301] Typo

11c1060

flothesof suggested changes Jan 17, 2020

View reviewed changes

VEZINET Didier and others added 4 commits January 17, 2020 15:51

[Issue301] Rename _DCONFIG_TABLE -> _DCONFIG_SHORTCUTS

ef8ffa1

[Issue301] Added comments (for dev) and docstr to new method, also in…

2b90034

…cluded new method in tf.geom.utils.__all__

[Issue301] Better formatted msg (url on its onw line)

f7444b9

Merge branch 'devel' into Issue301_GetListConfigs

66bc709

Didou09 requested a review from flothesof January 17, 2020 15:10

VEZINET Didier added 2 commits January 17, 2020 16:14

[Issue301] PEP8 compliance

56f3078

Merge branch 'Issue301_GetListConfigs' of https://github.com/ToFuProj…

7340194

…ect/tofu into Issue301_GetListConfigs

flothesof suggested changes Jan 17, 2020

View reviewed changes

None and True options for returnas are also handled in get_available_…

7a3aca1

…config() (default to str)

Didou09 merged commit 468e68a into devel Jan 17, 2020

Didou09 deleted the Issue301_GetListConfigs branch January 17, 2020 15:54

Didou09 mentioned this pull request Jan 17, 2020

accessing private method in tutorial #301

Closed

lasofivec mentioned this pull request Jan 21, 2020

Broken tutorial: default configuration #329

Closed

This was referenced Jan 23, 2020

Issue281 new default configs - AUG #333

Merged

tofu 1.4.2 preparation #339

Merged

Didou09 mentioned this pull request Mar 10, 2020

Preparing 1.4.3 #364

Merged

		return msg


		def get_available_config(dconfig=_DCONFIG, dconfig_table=_DCONFIG_TABLE,

Conversation

Didou09 commented Jan 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivations:

Main changes:

Example:

Issues:

Uh oh!

pep8speaks commented Jan 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Comment last updated at 2020-01-17 15:45:54 UTC

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Jan 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

flothesof left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flothesof left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Didou09 commented Jan 13, 2020 •

edited

Loading

pep8speaks commented Jan 13, 2020 •

edited

Loading

codecov-io commented Jan 14, 2020 •

edited

Loading