Vtk refactor #1229
Vtk refactor #1229
Conversation
| # other cases users may enter. What about: | ||
| # - Non-zero integers other than 1? | ||
| # - "yes", "Yes", "Y"? | ||
| return x in ("y", 1, True) |
There was a problem hiding this comment.
@doutriaux1 Should we be allowing these other cases to mean 'true'? They're fairly standard conventions, but I'm not sure what VCS expects. Is this information in the documentation?
There was a problem hiding this comment.
@dlonie most attribute setting go through VCSValidationFunction which takes any of the has an input and sets it to True anyway. I know of one case where somehow VisTrails bypass this and sets directly the _attribute and we endup with "False" (string not boolean) but this wouldn't cover it anyway. I think we should remove this test, and if anything breaks, make sure the attribute is actually set via VCSValidation function.
There was a problem hiding this comment.
as far as doc goes if we use VCSValidation functions, then a "bad" value triggers an error that list "correct" possible values.
There was a problem hiding this comment.
That sounds far more preferable to me. I seem to recall a lot of patches going in that would add explicit checks for these cases, but if all of that is handled by the validator, then awesome! I'll change these to just check if the variable is True and any issues that pop up will be treated a validation bugs.
There was a problem hiding this comment.
Looks like this will fail after just checking for True:
https://github.com/UV-CDAT/uvcdat/blob/master/Packages/vcs/Lib/VTKPlots.py#L889
There was a problem hiding this comment.
@dlonie you're right let's assume it is sanatize at nput time. If something breaks it's better anyway to fix the part where it should have been sanitized
There was a problem hiding this comment.
Ah, thanks -- just what I was looking for.
Right now we have:
checkTrueFalse
checkOnOff
checkYesNo
Would it make sense to replace all of these with a standard checkBoolean that tests all of these cases and sets to True/False?
There was a problem hiding this comment.
Not as part of this patch, but as a later issue? It seems these all do the same thing but in slightly different ways.
There was a problem hiding this comment.
Let's not touch these functions per say, we might need these for something else that really just need yesno (and not boolean) But you're right let's create an issue to create a unifying function that understands all these and sets to True/False, checkBoolean should really only check if the input is a boolean, let's think of a better name, I liked checkOnOff but that's already used.Maybe checkTrigger ?
There was a problem hiding this comment.
I'll add that when I make this change, and leave the other functions as-is. We can always port usages of them later as needed.
I'll add a checkBoolean that strictly checks True/False, and a checkFuzzyBoolean that permits the other representations.
|
@doutriaux1 will provide comments after we tag 2.2 today. @dlonie we would like to have it merged as soon as possible. |
|
Any progress on this? |
|
Looks like this is the week @dlonie |
| """Implementation of the Pipeline interface for VCS boxfill plots. | ||
|
|
||
| Internal variables: | ||
| - self._legend: Contour labels. |
There was a problem hiding this comment.
legend actually has another meaning in VCS vs "labels" the legend is used to display which lines represent what. If think "labels" or "ContourLabels" might be better here.
There was a problem hiding this comment.
Ah, this is the variable named legend in the original plot2D, so I just kept the name. I'll rename it.
There was a problem hiding this comment.
oh I see we're in boxfill... yes this is probably the strings/labels to draw on the legend, user-specified. I guess it's an ok name in this case, since we're talking about labels on legend....
|
@dlonie I'm going to test it a bit now that I have commented on your questions. LOTS of conflicts with master/release now... Are you going to bring everything back in? |
|
@doutriaux1 Don't worry too much about testing or resolving conficts at this point. I'll merge the changes in master back into this, and then we can do testing. After I get this resync'd with master, let's merge ASAP so we don't diverge too much again. |
|
@dlonie ok, but where to merge to? There's conflict with master and I don't think we want master to get this right now as it has not been thoroughly tested. release is not an option, @aashish24 is it time to create a "devel"/"experimental" branch (off off master/release) ? |
|
Definitely not release. I'd say testing and merging to master should be sufficient. There isn't any new behavior or changes to functionality here, all I've done is separate out the implementations and strip out the bits that would never execute for a given plot type. I've specifically kept significant changes out of this branch to preserve the behavior of master to make sure that we can get it in quickly. I'd rather not put this into a long-term branch off of master, as that is going to just keep diverging and require manual updating. Let's get this into master and then we can work on this going forward. |
|
@dlonie fine by me but @aashish24 didn't even want the release bits to go into master... I'm not quite sure what he has in mind for master... Can you please merge master AND release into this one (or a new branch) for now so that we can really test it. I really don't want master to get dirty that's what people clone right out of git. Thanks. |
|
Hmm. I'll just do master at the moment, and release when we merge release in. I wasn't aware that release was diverging from master like that. Typically changes that go into a release branch also go into master. @aashish24 What's going on with master? |
|
@dlonie @aashish24 tried to explained it to me there: #1259 |
This should probably replace the other similar functions for consistency on the backend, but this works for now.
This change shouldn't affect external behavior at this point, but just organizes the code and separates the logic for different plot types to make the implementations more robust to changes and easier to read/follow/debug. The Pipeline class is an interface for interacting with VTK. At this point, it defines a single virtual method, plot(...), which takes arguments similar to VTKPlots.plot(...). It still returns the undocumented map with objects that are reused for animations, so that code should continue to function as before. This patch: - Creates initial Pipeline interface. - Add shared logic from Plot2D into Pipeline2D subclass. - Adds specific implementations as Boxfill-, Isoline-, Isofill-, and MeshfillPipeline subclasses. - Cleans up style to conform to PEP 8. - Reraise exception in new boxfill test when it makes sense (made it easier to track down why the test was failing.) - Creates vcs2vtk.isTrue(...) to perform consistent user_input-->bool conversions.
|
Ok, new version is rebased on current master and incorporates feedback from @doutriaux1 👍 After review/testing, this should be ready to merge...somewhere, as soon as we figure out where that somewhere is! I still believe this should go into master. We don't want to merge this into release, as it shouldn't go into 2.2 -- it should go into the master branch as that's the target for non-release development. At least, this is how it typically works in other projects. Otherwise, we're essentially freezing master while we stabilize the release, which more or less defeats the purpose of the release branch. @aashish24 Can you clarify what goes where? |
|
this change should not go into release. This is a major change and should only be merged into master. Earlier, I have merged relevant changes into master from release to make sure that we have all the bug fixes in master as well. After this branch goes into master, any changes to these files will have to target particular branch. So in summary:
|
|
@aashish24 Thanks, sounds good. |
|
@aashish24 ok we all agree it should go into master only. |
|
ok all test pass for me let's do this! |
|
thanks @doutriaux1. @dlonie if we setup a mechanism to follow branch when we checkout testdata would that break anything? @doutriaux1 I will make sure that everything in release is in master as well. |
|
thanks @aashish24 |
Refactor VTKPlots.plot2D.
@doutriaux1 @chaosphere2112 Please read the following, and test/review the patch. I'll be asking specific questions inline, as there are some conventions used that I couldn't quite figure out from just reading the code/comments.
This change should neither affect nor improve external behavior at this point, but just organizes the code and separates the logic for different plot types to make the implementations more robust to changes and easier to read/follow/modify/debug.
The Pipeline class is an interface for interacting with VTK. At this point, it defines a single virtual method, plot(...), which takes arguments similar to VTKPlots.plot(...).
Eventually, the interface can grow to include methods for e.g. animation update, but for now it still just returns the map of objects, so that code should continue to function as before.
This patch:
-- Sometimes we check if a variable is true with
if var == 'y', sometimes withif var == 1, sometimes withif var in ('y', 1, True), and sometimes with justif var:. Let's start using this function to keep things consistent and working right the first time. This also adds the advantage of being able to fix bugs more easily -- for example, if all non-zero integers should mean true (rather than just1), we can easily update all usages by just changing the function's definition.Feel free to ask questions or make suggestions if there's something you don't like.
This will break several pending branches on UV-CDAT. Ping me on them if you want help porting them to the new framework after this topic lands in master.