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

Adjust use of GetDefaultOpenOptions in PlainText quickrecipes #19829

Open
brugger1 opened this issue Sep 18, 2024 · 18 comments
Open

Adjust use of GetDefaultOpenOptions in PlainText quickrecipes #19829

brugger1 opened this issue Sep 18, 2024 · 18 comments
Assignees
Labels
bug Something isn't working docs update to the documentation impact medium Productivity partially degraded (not easily mitigated bug) or improved (enhancement) likelihood medium Neither low nor high likelihood reviewed Issue has been reviewed and labeled by a developer
Milestone

Comments

@brugger1
Copy link
Collaborator

I was looking at the Python code example in PlainText file format section of the Getting Data into VisIt chapter and found what I that was incorrect code.

On further investigation, I found it was referencing a snippet of code from our test suite so it must work. The issue is that the function to get the file open options is GetDefaultFileOpenOptions, but the example code used GetDefaultOpenOptions. It turns out the test suite code defines a GetDefaultOpenOptions that calls GetDefaultFileOpenOptions. Someone viewing that example would never know that and if they tried to use the code it wouldn't work.

@brugger1 brugger1 added bug Something isn't working likelihood medium Neither low nor high likelihood impact medium Productivity partially degraded (not easily mitigated bug) or improved (enhancement) docs update to the documentation labels Sep 18, 2024
@markcmiller86
Copy link
Member

markcmiller86 commented Sep 18, 2024

I'm pretty sure I introduced that and there is a comment in plaintext.py above that definition...

#   
# Use this function with deepcopy to ensure we always start from a
# prestine default state for open options.
#
def GetDefaultOpenOptions():
    if not hasattr(GetDefaultOpenOptions, 'defaultOpenOptions'):
        GetDefaultOpenOptions.defaultOpenOptions = copy.deepcopy(GetDefaultFileOpenOptions("PlainText"))
    return copy.deepcopy(GetDefaultOpenOptions.defaultOpenOptions)

This is to ensure that each block of code starts from a fresh/clean copy of the options instead of having to constant re-define them from block to block.

Now, you've identified a big problem...the code appearing in the docs then doesn't look as it should. So, this can be corrected by first copying the method pointer, GetDefaultFileOpenOptions, to a local variable in the .py file and then overriding that method as below...

_getDefaultFileOpenOptions = GetDefaultFileOpenOptions
#
# Use this function to override GetDefaultFileOpenOptions with deepcopy to ensure
# we always start from a prestine default state for open options.
#
def GetDefaultFileOpenOptions(pluginName):
    assert(pluginName=='PlainText')
    if not hasattr(GetDefaultFileOpenOptions, 'defaultOpenOptions'):
        GetDefaultFileOpenOptions.defaultOpenOptions = copy.deepcopy(_getDefaultFileOpenOptions(pluginName))
    return copy.deepcopy(GetDefaultFileOpenOptions.defaultOpenOptions)

Then everywhere in the file replace GetDefaultOpenOptions() (which is misleading) to GetDefaultFileOpenOptions('PlainText') which is the code as it should appear. Note, I added an assertion in the above to enforce that it is always called that way.

This is some of the gymnastics involved in including code in our docs that actually also gets run and passes its testing. But, I think it is worth doing too. In this case each example block should stand on its own in the documentation and not have remnents having to do with undoing some of the effects from other blocks.

@JustinPrivitera
Copy link
Member

Didn't this get addressed here: #19663

@JustinPrivitera
Copy link
Member

I am wondering where that fix went

@markcmiller86
Copy link
Member

It is committed on develop and the remarks you added do appear in the docs here, https://visit-sphinx-github-user-manual.readthedocs.io/en/develop/data_into_visit/PlainTextFormat.html

That said, I much prefer the solution I proposed here because it results in the code blocks appearing exactly as they would and removes the need for any commentary to explain it.

Maybe I should take this one?

@JustinPrivitera
Copy link
Member

What is the stable version of the docs? How do you get there?
image

@markcmiller86
Copy link
Member

@JustinPrivitera can you go to readthedocs.org and register for a free account there? Then, I can add you to the VisIt documentation generation project.

@JustinPrivitera
Copy link
Member

ok will do.

The URL that Eric sent with the bad docs is from stable, the develop docs are fine. What are the "stable" docs and how do you get to them? How would users get to them?

@JustinPrivitera
Copy link
Member

I made an account with the username privitera1

@markcmiller86
Copy link
Member

I sent you an invite for the VisIt project there.

I have no idea what stable on ReadTheDocs actually means. It don't see any way to tell RTD what branch stable should be associated with.

@JustinPrivitera
Copy link
Member

It is committed on develop and the remarks you added do appear in the docs here, https://visit-sphinx-github-user-manual.readthedocs.io/en/develop/data_into_visit/PlainTextFormat.html

That said, I much prefer the solution I proposed here because it results in the code blocks appearing exactly as they would and removes the need for any commentary to explain it.

Maybe I should take this one?

So regardless of it we go with your solution (which we probably should) or not, the problem will still remain that your fix will go on develop, and stable will stay what it is. I don't understand why stable exists nor how you get there without manually choosing it when looking at the docs. Perhaps @brugger1 can shed some light.

@JustinPrivitera
Copy link
Member

I joined the visit project on read the docs; thanks for the invite. It looks like I can hide stable if I choose:
image

3.4.1 seems to also use stable:
image

This is all very mysterious.

@markcmiller86
Copy link
Member

yes, but hiding stable has other downsides...https://docs.readthedocs.io/en/stable/versions.html#version-states

@markcmiller86
Copy link
Member

@JustinPrivitera
Copy link
Member

I see. So next time we do a major release, stable will get all the changes we've added to develop.

@JustinPrivitera
Copy link
Member

To the original point of this ticket, @brugger1's concern has already been addressed here: #19669

To @markcmiller86's suggestion, it wouldn't be a bad idea to make changes such that things were even clearer in the docs.

@markcmiller86 markcmiller86 self-assigned this Sep 18, 2024
@markcmiller86 markcmiller86 added this to the 3.4.2 milestone Sep 18, 2024
@markcmiller86
Copy link
Member

I personally find it a bit onerous to have to two branch creations, a cherry-pick or a patch, and two pull requests for documentation updates. So, my desire is to do documentation work in just one place. That means develop. And, I know that develop is always visible to our customers. I've done documentation work on the release candidate only when I thought the material was somewhat unique to features of the RC.

I have never bothered to use stable. But, I think our customers might...especially if that is the ReadTheDocs way. So, given what we've just learned about what stable on ReadTheDocs means, I wonder if I/we should adopt a different strategy?

@JustinPrivitera
Copy link
Member

I want to consult with the experts. Cyrus runs docs for multiple projects and might have more answers. Eric may have more info (?). Maybe we discuss tomorrow morning?

@JustinPrivitera
Copy link
Member

discussion results here: #19835

@markcmiller86 markcmiller86 modified the milestones: 3.4.2, 3.4.3 Sep 26, 2024
@cyrush cyrush added the reviewed Issue has been reviewed and labeled by a developer label Sep 26, 2024
@markcmiller86 markcmiller86 changed the title Issue with opening PlainText file example in Getting Data into VisIt. Adjust use of GetDefaultOpenOptions in PlainText quickrecipes Sep 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working docs update to the documentation impact medium Productivity partially degraded (not easily mitigated bug) or improved (enhancement) likelihood medium Neither low nor high likelihood reviewed Issue has been reviewed and labeled by a developer
Projects
None yet
Development

No branches or pull requests

4 participants