-
Notifications
You must be signed in to change notification settings - Fork 3
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
Improve algorithm and typing of ActorNeutronics #167
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bclyons12 you raise a good point here about the type of
IMAS.dd
. This is something I had not originally caught!In addition of having to define the actors constructors as you have done, we should do the same for all functions that use type annotations. That is ~500 hits across 58 files. Doable but still annoying.
I'll merge this, and actually remove the
{T}
from the constructor (I just don't like to have the source of an actor being different than the others. I'll open a separate issue specifically to discuss this and decide a course of action.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@orso82 You need to do this for structs, since
IMAS.dd
is abstract. You don't need to do it for function arguments though. SinceIMAS.dd{T} <: IMAS.dd
, having::IMAS.dd
will allow the compiler will dispatch on each separately.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, ok! you are right! Phew 😅
What threw me off is that you added this constructor.
Is this really necessary? shouldn't
ActorNeutronics
pick up the right{T}
already based on the concreteIMAS.dd
type that is passed?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only if you don't override the default inner constructor. You have here (something I advocate against 😃 ), so it doesn't make the default outer constructor either.
Here's the relevant bit from https://docs.julialang.org/en/v1/manual/constructors/ :
![Screen Shot 2022-10-11 at 9 25 45 PM](https://user-images.githubusercontent.com/1216060/195249871-9e72839c-0940-45be-a547-abe3cfa47655.png)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You actually might need the inner constructor here, since you use it to accept keywords and Julia doesn't multiple dispatch on keywords. If you made that as an outer constructor, it couldn't distinguish between that and the default inner constructor...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, that's also something I had not caught.
As you noted, all actors that only have
dd
andpar
as their structure need to use a inner constructor to accept keywords arguments. For all other actors I have defined an outer constructor.