Closed ewu63 closed 3 years ago
Is this PR ready for review? Will the things in the other PR be moved to this PR too?
Is this PR ready for review? Will the things in the other PR be moved to this PR too?
Up to the maintainers I think, I can go either way but maybe it's a bit better to do the rest in a separate PR? That way this is mostly just moving files around.
Is this PR ready for review? Will the things in the other PR be moved to this PR too?
Up to the maintainers I think, I can go either way but maybe it's a bit better to do the rest in a separate PR? That way this is mostly just moving files around.
I agree that keeping things separate is a good thing, since the other items complete tasks that aren't reorganizing the files. @joanibal what do you think?
This generally looks good to me. Github does not seem to match up the files, so it is unclear if the line history will be preserved across the squashed commit when visualizing the changes on Github.
The history seems to be preserved, see for example here. I don't think squash will change anything.
I'm willing to work on the remaining items after this is merged. As for further refactoring, we may want to identify specific functions that could be re-used elsewhere, and make sure the code is compatible. Then we will have to identify a good place to put the shared code. This is potentially a much bigger task since there is significant code duplication across several repos (code working with CGNS files, or geometric operations etc).
I also re-ran the failed pipeline which now passes with the updated intel image.
This generally looks good to me. Github does not seem to match up the files, so it is unclear if the line history will be preserved across the squashed commit when visualizing the changes on Github.
The history seems to be preserved, see for example here. I don't think squash will change anything.
I'm willing to work on the remaining items after this is merged. As for further refactoring, we may want to identify specific functions that could be re-used elsewhere, and make sure the code is compatible. Then we will have to identify a good place to put the shared code. This is potentially a much bigger task since there is significant code duplication across several repos (code working with CGNS files, or geometric operations etc).
I also re-ran the failed pipeline which now passes with the updated intel image.
Great, that looks good. It's confusing that Github doesn't show it like that in the normal diff, must just be an interface issue.
Thanks for working on the refactoring, that sounds good to me. I agree that moving code between repos (or to a new repo) is a much bigger task. If / when we do go about doing that, we likely will want to look at all the repos for duplicate or useful code, not just pySpline alone (as you said).
@joanibal could you take a look when you have a chance?
Looks good.
I agree that it would be a good idea to split the tecplot utilities into a separate repo at some point.
Purpose
I did it properly this time (unlike #20) so all the line histories are preserved. Note that this should not cause problems for users since regular imports such as
from pyspline import Surface
will work as before (since we updated theinit
).Things from the other PR that are not in this PR yet:
np
These could be either added now, or done in a separate PR. I'm fine with either really.
Type of change
What types of change is it? Select the appropriate type(s) that describe this PR
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.flake8
andblack
to make sure the code adheres to PEP-8 and is consistently formatted