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
Update VTK #2146
Update VTK #2146
Conversation
Data in VCS in: VTK conda recipes:
|
Fixes: |
@doutriaux1 @aashish24 Please review. Especially pay attention to the 400+ baseline changes. Not sure how do we update VTK. I included the md5 for the VTK I tested with (from yesterday). |
9f87557
to
9e050b9
Compare
@doutriaux1 @aashish24 Ping! |
@danlipsa we should look into the newer flake8, probably just a matter of adding a few rules to the excepted ones. |
@doutriaux1 OK, I'll fix those. |
@doutriaux1 They all pass. flake8 was not installed in my conda env. |
@doutriaux1 @aashish24 Lets merge this! :-) The last test reason is: |
@doutriaux1 @aashish24 I can do it myself, I just need the go ahead! |
+2 :LGTM |
@chaosphere2112 @aashish24 I'm offline for another day at least, please take a careful look and let's have @chaosphere2112 merge in. |
@doutriaux1 I would prefer the developers merge their branch. I am fine with @chaosphere2112 review the branch. This is master branch and not a release. if someone finds a new bug which is not captured by our testing then we can always fix them later. Let's merge it on Thursday as I need this branch for some of my changes. |
@danlipsa I believe you will push another branch with GL2PS fixes after this one? Also, is travis broken ever since we moved our vcs to separate repo? |
@doutriaux1 @chaosphere2112 Guys, any progress on reviewing this? My finger itches to press "Merge pull request". |
@danlipsa I will review the baselines tomorrow and will run the tests on my linux. If both looks good to me, I will merge it. |
Unless someone else beats me to it. |
@danlipsa what's your process of running your tests right now? |
This is what I use. Not all steps are necessary once are done the first time. cd projects Create uvcdat repo and build the conda envmkdir uvcdat See the name of the created conda envconda env list Delete the created conda env and create another one with the same name using conda-forgeconda env remove -n v2.8-1-gbf17786 Clone vcs repo and overwrite vcs in the conda env.cd .. Rebuild a new VTK and instal itUpdate the location for VTK Run the testscd ../../uvcdat/build |
thanks @danlipsa |
@aashish24 Note I added a step for rebuilding VTK |
@danlipsa I am at the last step of building everything, hoping to publish my results soon. |
@danlipsa the new test_vcs_basic_boxfill_aeqd_proj_SH.png looks little off. Mind having a look at it? test_vcs_basic_boxfill_aeqd_proj_SH.png |
@danlipsa I would suggest comparing the baselines in some viewer: I wrote this script (below) to do that
|
@aashish24 Yes, I am doing something similar. Do you see this difference on Linux, or a Mac? |
@aashish24 All these tests pass in 2.8 because of a _1 version with the blue-yellow color map. Now the _1 version was promoted to the main version as it should. |
@aashish24 @doutriaux1 Can I merge this? |
reviewing this afternoon. |
Sounds good. |
Let's give @doutriaux1 this afternoon to review 🤓 but I would suggest we merge it tomorrow morning. We need to move forward with some branches so unless we find any critical issue with this branch we merge it tomorrow. |
Since github.com/CDAT/vcs/pull/112 is already merged i am merging this as well |
@danlipsa shouldn't there be a vtk pr to merge as well? Also travis still fails.... |
@doutriaux1 the first comment has the SHA and url for the VTK I used in the tests. Do we need to merge anything in or we can use that in conda-receipes? I'll have to merge in the data as well. |
@doutriaux1 @aashish24 conda-recipes/vtk-cdat/meta.yaml.in has This includes the changes we want, but it is a moving target. I think we should use a specific SHA we tested with. |
+1 I was wondering about it as well. |
@danlipsa we need to update our vtk repo and build a nightly. |
@danlipsa I will update .travis.yaml to use nightly builds |
@doutriaux1 Not sure I follow. What is our VTK repo? As I said conda-recipes/vtk-cdat/meta.yaml.in points to git://github.com/Kitware/VTK.git, master now. |
@doutriaux1 we need to use a SHA vs the master. Using master of VTK repo is risky in some ways. |
@aashish24 @danlipsa we have https://github.com/uv-cdat/VTK just for that purpose. @aashish24 created this. uvcdat-master is the branch that we should use for nightly I retag uvcdat-master wth each release. |
@doutriaux1 @aashish24 OK, so I'll point that branch to the VTK SHA I tested with. We should probably change conda-recipes/vtk-cdat/meta.yaml.in to point to that as well isn't it? |
@doutriaux1 @aashish24 I updated uvcdat-master on https://github.com/uv-cdat/VTK to point to the SHA I tested (VTK from Dec 22). @doutriaux1 Are you going to update conda-recipes or you want me to do it? |
@danlipsa conda-recipe should point to uvcdat-master of https://github.com/uv-cdat/VTK which at this point I think should really be just master I dont see the point of having a uvcdat-master branch. |
@danlipsa so I don't think conda-recipe needs an update. |
@doutriaux1 We got used with uvcdat-master, so maybe we should leave it as is. Also master could be interpreted as VTK master which is not. So I vote to keep using uvcdat-master. |
@danlipsa @doutriaux1 I think if we use uvcdat/vtk, using master is fine since we will only update the master when we know vtk original master is in good condition for UV-CDAT> |
@danlipsa @aashish24 I'm fine either way. Let's revisit this next time we merge something from VTK into our VTK repo. |
No description provided.