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

Rhino compatibility #31

Merged
merged 19 commits into from
Apr 26, 2024
Merged

Rhino compatibility #31

merged 19 commits into from
Apr 26, 2024

Conversation

chenkasirer
Copy link
Collaborator

@chenkasirer chenkasirer commented Mar 5, 2024

Some minor changes related to Rhino/Ironpython compatibility.

  • Added Rhino install plugin
  • Inserted numpy imports into the methods where they are called
  • Added PlateElement and PlateFeature to second level imports

What type of change is this?

  • Bug fix in a backwards-compatible manner.
  • New feature in a backwards-compatible manner.
  • Breaking change: bug fix or new feature that involve incompatible API changes.
  • Other (e.g. doc update, configuration, etc)

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.

  • I added a line to the CHANGELOG.md file in the Unreleased section under the most fitting heading (e.g. Added, Changed, Removed).
  • I ran all tests on my computer and it's all green (i.e. invoke test).
  • I ran lint on my computer and there are no errors (i.e. invoke lint).
  • I added new functions/classes and made them available on a second-level import, e.g. compas.datastructures.Mesh.
  • I have added tests that prove my fix is effective or that my feature works.
  • I have added necessary documentation (if appropriate)

@petrasvestartas
Copy link
Collaborator

petrasvestartas commented Mar 11, 2024

I reviewed the commits. I think, it can be merged.

I would maybe just keep model.print(), instead of model.print_model()

Or, if we specify the type of print, then it would be good to have print_hierarchy(), print_interactions() that refers to a) whole model, b) tree, c) graph. But I think you most often want to do this: model.print() or print(model) in the shortest way possible.

@chenkasirer
Copy link
Collaborator Author

I reviewed the commits. I think, it can be merged.

I would maybe just keep model.print(), instead of model.print_model()

Or, if we specify the type of print, then it would be good to have print_hierarchy(), print_interactions() that refers to a) whole model, b) tree, c) graph. But I think you most often want to do this: model.print() or print(model) in the shortest way possible.

Hey @petrasvestartas!

Thanks for reviewing. The print methods I changed because IronPython for some reason won't let you define a method called print not sure why:

IronPython 2.7.8 (2.7.8.0) on .NET 4.0.30319.42000 (64-bit)
Type "help", "copyright", "credits" or "license" for more information.
>>> def print():
  File "<stdin>", line 1
    def print():
        ^
SyntaxError: unexpected token 'print'

@chenkasirer
Copy link
Collaborator Author

I reviewed the commits. I think, it can be merged.
I would maybe just keep model.print(), instead of model.print_model()
Or, if we specify the type of print, then it would be good to have print_hierarchy(), print_interactions() that refers to a) whole model, b) tree, c) graph. But I think you most often want to do this: model.print() or print(model) in the shortest way possible.

Hey @petrasvestartas!

Thanks for reviewing. The print methods I changed because IronPython for some reason won't let you define a method called print not sure why:

IronPython 2.7.8 (2.7.8.0) on .NET 4.0.30319.42000 (64-bit)
Type "help", "copyright", "credits" or "license" for more information.
>>> def print():
  File "<stdin>", line 1
    def print():
        ^
SyntaxError: unexpected token 'print'

Asked ChatGPT which reminded me that in Python 2.x print is a language token, not just a function. So I guess that's why..

@@ -117,7 +116,7 @@ def __getitem__(self, index):
# Info
# ==========================================================================

def print(self):
def print_model(self):
Copy link
Collaborator

Choose a reason for hiding this comment

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

perhaps we should just switch to using __str__ and print with print(model)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sounds good to me but would have to make changes in Tree.print_hierarchy and InteractionGraph.print_interactions as they internally call print

Copy link
Collaborator Author

@chenkasirer chenkasirer Mar 11, 2024

Choose a reason for hiding this comment

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

btw I think we should generally avoid calls to print in COMPAS, especially the parts that are more library-ish.

Copy link
Collaborator

@petrasvestartas petrasvestartas Mar 14, 2024

Choose a reason for hiding this comment

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

Let's change to __str__ and then additional change will be needed for the tree in compas and merge this pull request. Due to multiple other packages, still ironpython needs to be supported for some time.

@chenkasirer
Copy link
Collaborator Author

@tomvanmele changed prints to __str__. This relies on the changes by @Licini in compas-dev/compas#1314.

src/compas_model/elements/block.py Show resolved Hide resolved
src/compas_model/elements/interface.py Show resolved Hide resolved
@@ -84,7 +91,8 @@ def edge_interaction(self, edge):
"""
return self.edge_attribute(edge, "interaction") # type: ignore

def print_interactions(self):
def interactions(self):
Copy link
Collaborator

Choose a reason for hiding this comment

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

i dont' think this makes sense.InteractionGraph.interactions should not return a string, but rather a list of interactions.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

agree that's weird, but this previously did the same just using print. I renamed this to interactions_str as it really just builds a string form of the interactions.

What would you have InteractionGraph.interactions return? just a flat list of Interactions?

Copy link
Collaborator

Choose a reason for hiding this comment

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

yes, a flat list of all interaction objects for easy access. we do the same, for example, with elements in the model...

src/compas_model/model/model.py Outdated Show resolved Hide resolved
src/compas_model/model/model.py Outdated Show resolved Hide resolved
@@ -84,8 +91,16 @@ def edge_interaction(self, edge):
"""
return self.edge_attribute(edge, "interaction") # type: ignore

def print_interactions(self):
"""Print the interactions contained in the graph."""
def interactions_str(self):
Copy link
Collaborator

Choose a reason for hiding this comment

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

in my opinion this long string should be built by __str__ itself, or by some kind of private method, but not by a public API method, since it is basically a utility of __str__...

@petrasvestartas
Copy link
Collaborator

@chenkasirer Invoke format and push once more.

@petrasvestartas
Copy link
Collaborator

@tomvanmele

Could we merge and close this pull request?

@tomvanmele tomvanmele merged commit ca2e26a into BlockResearchGroup:main Apr 26, 2024
11 checks passed
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