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

CEP27: toolkit Snippet #297

Merged
merged 21 commits into from
Mar 27, 2021
Merged

CEP27: toolkit Snippet #297

merged 21 commits into from
Mar 27, 2021

Conversation

bam241
Copy link
Member

@bam241 bam241 commented Oct 30, 2019

This is a suggestion to improve, ease and homogenize an the usage of toolkit capability in archetypes.

This idea is to limit as much as possible the need for implementation in a newly develop archetypes to avoid divergence in the implementation and the convention by providing a snippet that one can simply includes in the archetypes class definition header (inside the core definition of the class) to ensure variable name are identical.

Example can be found in:

@bam241 bam241 requested a review from gonuke October 30, 2019 15:45
@bam241 bam241 assigned bam241 and unassigned bam241 Oct 30, 2019
@bam241 bam241 requested a review from katyhuff October 30, 2019 15:45
Copy link
Member

@gonuke gonuke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A bunch of editorial changes - I'm not sure I fully understand all the usage details, either.

source/cep/cep27.rst Outdated Show resolved Hide resolved
source/cep/cep27.rst Outdated Show resolved Hide resolved
source/cep/cep27.rst Outdated Show resolved Hide resolved
source/cep/cep27.rst Outdated Show resolved Hide resolved
source/cep/cep27.rst Outdated Show resolved Hide resolved
source/cep/cep27.rst Outdated Show resolved Hide resolved
source/cep/cep27.rst Outdated Show resolved Hide resolved
source/cep/cep27.rst Outdated Show resolved Hide resolved
source/cep/cep27.rst Outdated Show resolved Hide resolved
2. Add the proper default initialisation of the variable required for the
snippet.

3. In the ``Archetype::EnterNotify()``, assign the Snippet with value
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you really assign the "Snippet" with a value???

@bam241
Copy link
Member Author

bam241 commented Nov 11, 2019

Cc @yarden-livnat

Copy link
Member

@gonuke gonuke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This nearly there - thanks for the updates.

source/cep/cep27.rst Outdated Show resolved Hide resolved
source/cep/cep27.rst Outdated Show resolved Hide resolved
source/cep/cep27.rst Outdated Show resolved Hide resolved
add characteristics to archetypes such as Position or Metadata, the actual
implementation method is very verbose, where in each archetypes one needs to add
the new specification in the arcehtype header, then assign it and use it in the
cpp file, with no guarantee on the consistency in the variable naming and
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sentence kind of runs on... Maybe consider making it a couple of sentences.

source/cep/cep27.rst Outdated Show resolved Hide resolved
source/cep/cep27.rst Outdated Show resolved Hide resolved
source/cep/cep27.rst Outdated Show resolved Hide resolved
source/cep/cep27.rst Outdated Show resolved Hide resolved
source/cep/cep27.rst Outdated Show resolved Hide resolved
source/cep/cep27.rst Outdated Show resolved Hide resolved
var1(0.0),
var2(0.0),
...,
coordinates(0,0), //coordinates constructor (toolkit feature class)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This initializer is not necessary/valid because the snippet in this example doesn't include the coordinates variable.

--------------------
``toolkit/my_snippet.cycpp.h``:
.. highlight:: c
cyclus::toolkit::Position coordinates;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this only in the version without inheritance? Is this the only difference between the with and without?

Copy link
Member Author

@bam241 bam241 Nov 27, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it also changes in the implementation of the archetype

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean the only difference in the snippet. It would be great if the same snippet could be used in both modes.

var1(0.0),
var2(0.0),
...,
coordinates(0,0), //coordinates constructor (toolkit feature class)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we use C++11 can we initialize these in the snippet?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes that's true, this will be way better !

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll try this in my Position PR and see how it does work out!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it seems to be working, I updated both the PR and the CEP accordingly

source/cep/cep27.rst Show resolved Hide resolved

The |Cyclus| toolkit is designed to easily implement specific capabilities in
newly developed archetypes, such as a trading policy, commodity producers, etc. To
add characteristics to archetypes such as `Position` or `Metadata`, the actual
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is still a long, awkward sentence.

--------------------
``toolkit/my_snippet.cycpp.h``:
.. highlight:: c
cyclus::toolkit::Position coordinates;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean the only difference in the snippet. It would be great if the same snippet could be used in both modes.

Copy link
Member

@gonuke gonuke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry - just one more suggestion...

And then we need review from others!

source/cep/cep27.rst Outdated Show resolved Hide resolved
bam241 and others added 2 commits November 27, 2019 17:05
…ader, and renameing my_snippet into my_feature_snippet
Co-Authored-By: Paul Wilson <paul.wilson@wisc.edu>
Copy link
Member Author

@bam241 bam241 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also renamed my_snippet into my_feature_snippet.
I believe it was making more sense.

source/cep/cep27.rst Show resolved Hide resolved
@gonuke
Copy link
Member

gonuke commented Nov 27, 2019

I still wonder if there is a way for the same snippet to be used for inheritance and membership?

@bam241
Copy link
Member Author

bam241 commented Nov 27, 2019

I still wonder if there is a way for the same snippet to be used for inheritance and membership?

that would be nice !

maybe an other way to do this would be to let cycpp do that using #pragmas ?

@gonuke
Copy link
Member

gonuke commented Nov 27, 2019

Maybe we don't include the member variable in the snippet? The user needs to define the name for that member variable? That relaxes one kind of imposed consistency, but maybe that variable name is purely internal and doesn't need to be consistent... just a thought

@bam241
Copy link
Member Author

bam241 commented Nov 27, 2019

Maybe we don't include the member variable in the snippet? The user needs to define the name for that member variable? That relaxes one kind of imposed consistency, but maybe that variable name is purely internal and doesn't need to be consistent... just a thought

I liked the snippet idea for the variables exposed in the cyclus input to ensure those one are the same

--- edit ---
I am not sure but I feel like I am missing something about your idea !

--------------------
``toolkit/my_feature_snippet.cycpp.h``:
.. highlight:: c
cyclus::toolkit::Position coordinates(0,0);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you remove only this line (not exposed in input), then the snippet can be used in both modes, and developer chooses name of member variable.

In fact, that might be better, because otherwise developer has to know name of member variable defined in the snippet.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is correct, (and I like the versatility it provides)
I guess it is a balance between versatility and amount of work on the developer side:
if we only support without-inheritance everything should be includable in the snippet.
if we want to support both, developers have to add:

  • #include 'toolkit/my_toolkit.h
  • inheritance or feature element
  • #include toolkit/snippet.cycpp.h` in the class core.

(this is also why without inheritance has my (slight) preference.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the good thing with removing the variable it would allow both with and without inheritance

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even without inheritance, I'm not sure we want to assume that the member variable is always defined by the snippet... what if you want two positions?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure but then what is the use of the snippet ? because some member variable are defined in the snippet (at least the one expose in the input.)

I don't see how to use the snippet with 2 positions.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might not be the best example of including more than one instance. In addition, there may be a need to initialize each instance which gets complicated again. Nevertheless, I think asking the developer to explicitly declare their member variable is not too much to ask.

We should open this up to additional discussion, e.g. on the mailing list.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still think there is a benefit to having a single snippet file and then letting user choose the variable name in the non-inheritance case. We can include that in the example in the documentation.

Once we get that resolved, and clean up this CEP, I think I'm ready to approve/merge this and the follow-up PR's in the code repos.

source/cep/cep27.rst Outdated Show resolved Hide resolved
source/cep/cep27.rst Outdated Show resolved Hide resolved
source/cep/cep27.rst Outdated Show resolved Hide resolved
@katyhuff
Copy link
Member

Just a couple little grammar comments suggested above -- otherwise looks good to me.

Co-Authored-By: Katy Huff <katyhuff@users.noreply.github.com>
Copy link
Member

@gonuke gonuke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is ready to move forward

@gonuke gonuke merged commit 06bd7ea into cyclus:source Mar 27, 2021
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.

3 participants