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

Fix This instantiation #56

Merged
merged 4 commits into from
Mar 24, 2021
Merged

Fix This instantiation #56

merged 4 commits into from
Mar 24, 2021

Conversation

varunagrawal
Copy link
Collaborator

We can now use This with non-templated classes as well.

I am not sure if that is what @ProfFan was hinting at with the last task item in borglab/gtsam#407 but it works for both templated an non-templated classes. 🙂

@varunagrawal varunagrawal self-assigned this Mar 24, 2021
@varunagrawal
Copy link
Collaborator Author

Still need to add a test.

@varunagrawal
Copy link
Collaborator Author

Let's land #55 before this since then I can update the tests accordingly.

Copy link
Member

@dellaert dellaert left a comment

Choose a reason for hiding this comment

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

Maybe a bit more docs now that you're in this

class_instantiated_methods = list(original.methods)
self.properties = list(original.properties)
else:
if original.template:
Copy link
Member

Choose a reason for hiding this comment

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

Now that you know what this code does, and logic below, add a comment here to that effect.

self.name = instantiate_name(
original.name, instantiations) if not new_name else new_name

# Check for typenames if templated.
Copy link
Member

Choose a reason for hiding this comment

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

nice!

Instantiate the class constructors.

Args:
typenames: List of template types to instantiate.
Copy link
Member

Choose a reason for hiding this comment

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

Instantiate static methods in the class.

Args:
typenames: List of template types to instantiate.
Copy link
Member

Choose a reason for hiding this comment

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

"""
This function only instantiates class templates in the methods.
Template methods are instantiated in InstantiatedMethod in the second
round.

Args:
typenames: List of template types to instantiate.
"""
Copy link
Member

Choose a reason for hiding this comment

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

etc..


Args:
typenames: List of template types to instantiate.
"""
Copy link
Member

Choose a reason for hiding this comment

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

@varunagrawal
Copy link
Collaborator Author

Honestly, there is so much more documentation left to be done. I'll add more.

@varunagrawal
Copy link
Collaborator Author

Added the test, will add docs after I get some physical activity.

@varunagrawal
Copy link
Collaborator Author

Added docs. Merging.

@varunagrawal varunagrawal merged commit a95429e into master Mar 24, 2021
@varunagrawal varunagrawal deleted the fix/this-instantiation branch March 24, 2021 22:20
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