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

added child warning example #55

Merged
merged 7 commits into from
Jan 23, 2021
Merged

Conversation

joanibal
Copy link
Collaborator

Purpose

Added an example test to show some non-intuitive child FFD behavior

Type of change

What types of change is it?
Select the appropriate type(s) that describe this PR

  • Other (please describe)

Testing

Explain the steps needed to test the new code to verify that it does indeed address the issue and produce the expected behavior.

Checklist

Put an x in the boxes that apply.

  • I have run flake8 and black to make sure the code adheres to PEP-8 and is consistently formatted
  • I have run unit and regression tests which pass locally with my changes
  • I have added new tests that prove my fix is effective or that my feature works
  • I have added necessary documentation

Copy link
Collaborator

@bbrelje bbrelje left a comment

Choose a reason for hiding this comment

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

@joanibal, first of all, I am sorry for the delay. It's not excusable.

Are you intending this example to be part of the reg test suite? I checked the logs and I didn't see that test_parent_shape_child_rot() is being run. I think that the warning_childFFD.py needs to be renamed to begin with the word test_ in order for pytest to pick it up.

@anilyil
Copy link
Contributor

anilyil commented Jan 18, 2021

The changes look good. I would say we need to document this far better. I only know about this issue because we discussed it in the office. I think we should include an in depth explanation of this issue in the code. I dont remember the details 100% but I can help with this if you refresh my memory a bit. We should also put in comments in the childffd code pointing to this test and this behavior, so people who want to use that feature will be aware of this.

@joanibal
Copy link
Collaborator Author

@bbrelje I didn't intend it to be run from the reg test suite.

@joanibal
Copy link
Collaborator Author

@anilyil
I agree, I added some detail in issue #7.
Do you think we should move this info else where?

bbrelje
bbrelje previously approved these changes Jan 18, 2021
@bbrelje
Copy link
Collaborator

bbrelje commented Jan 18, 2021

I'm good with this one, then. Never used the child FFD feature so I didn't understand the backstory here.

@anilyil
Copy link
Contributor

anilyil commented Jan 18, 2021

@joanibal Can we refer to issue #7 in the code also? Finally, I think the figures on the issue can be improved. They are difficult to understand because too many things going on. The cube example you used to explain the issue to me in FXB was great. I am guessing the script in this PR also uses that example. Would it be too much work to add those figures to the issue?

@joanibal
Copy link
Collaborator Author

@anilyil
I added a reference to the issue in the code.

I will update the issue later to use a simpler example.

@anilyil anilyil requested a review from bbrelje January 21, 2021 11:10
@bbrelje bbrelje merged commit 45c4c2e into mdolab:master Jan 23, 2021
bernardopacini pushed a commit to bernardopacini/pygeo that referenced this pull request Mar 17, 2021
* added child warning example

* formatting changes

* added unittest.main

* tweaked unittest.main

* tweaked unittest.main again

* add issue ref in code
@marcomangano marcomangano mentioned this pull request Mar 18, 2021
11 tasks
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