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

Docstring - HeatCharge docstring error #1798

Merged
merged 1 commit into from
Jul 5, 2024
Merged

Conversation

marc-flex
Copy link
Contributor

This PR fixes one docstring error. The test still doesn't pass because some routines raise warnings.

These warnings are there by design so we may want to consider relaxing this test so that warnings are ignored? @daquintero @momchil-flex

@marc-flex marc-flex added the 2.8 will go into version 2.8.* label Jul 4, 2024
@marc-flex marc-flex requested a review from yaugenst-flex July 4, 2024 06:20
Copy link
Collaborator

@yaugenst-flex yaugenst-flex left a comment

Choose a reason for hiding this comment

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

Great, thanks @marc-flex! I agree doctests probably shouldn't fail on warnings, I'm not sure if there is a good way to do that? I've only been able to find things that pollute the example code itself, which is not great.

@momchil-flex
Copy link
Collaborator

Yeah I think unfortunately right now we've been resorting to skipping such tests. At least they've been manually verified to work now... You can add # doctest +SKIP to every line that should be skipped. I don't know if there is a way to skip a whole block @yaugenst-flex ?

@yaugenst-flex
Copy link
Collaborator

yaugenst-flex commented Jul 4, 2024

Unfortunately I don't think that exists. A trick would be to define the whole block inside a function and then skip only the function call, i.e.

def f():
    ...  # do stuff here
f()  # doctest +SKIP

But I don't think we want our examples to look like this...

We could consider using xdoctest or pytest-doctestplus maybe, which both support that.

@yaugenst-flex
Copy link
Collaborator

yaugenst-flex commented Jul 4, 2024

Oh wait, nevermind actually, from the doctestplus page:

.. doctest-skip::

    >>> import asdf
    >>> asdf.open('file.asdf')

That might just work?

edit: it doesn't :/

@daquintero
Copy link
Contributor

daquintero commented Jul 4, 2024

Sorry on the delay on checking this, I believe I configured the docstring tests on the pytest section of the pyproject.toml.

tidy3d/pyproject.toml

Lines 244 to 258 in 92dc295

[tool.pytest.ini_options]
addopts = "--doctest-modules"
doctest_optionflags = "NORMALIZE_WHITESPACE ELLIPSIS"
norecursedirs = [
"tests/_test_local",
"tests/test_cli",
"tests/_test_data",
"tests/_test_notebooks",
"tidy3d/web",
"docs/notebooks",
"docs/faq",
]
filterwarnings = "ignore::DeprecationWarning"
testpaths = ["tidy3d", "tests", "docs"]
python_files = "*.py"

There might be an option to ignore the warnings possibly in the doctest-optionflags, just checking. Actually filterwarnings section could be used possibly.

@yaugenst-flex
Copy link
Collaborator

It's not really about ignoring warnings though (although that doesn't exist either I think), but the fact that these warnings are logged to stdout, and doctest expects all code and output to be included in the example block, otherwise it raises a DocTestFailure. If we just put the warning text on the next lines, then the test passes. I've played around with ELLIPSIS a bit but couldn't get that to work. And this also hurts example readability so maybe not a great solution even it it works...

@daquintero
Copy link
Contributor

daquintero commented Jul 4, 2024

Yeah agreed. Personally it'd be pretty good if we can update all our docstring examples do be self contained so that it's just a case of copy-pasting for the user and this way we can actually validate it runs properly. However, it's a bigger effort.

In any case for warnings, there's the filterwarnings pyproject.tomloption, but that's unlikely to fully sort it as you mention.

Skip tests with warning
@marc-flex marc-flex force-pushed the marc/docstring-fix branch from b027ac4 to 3d76674 Compare July 4, 2024 14:37
@marc-flex
Copy link
Contributor Author

Thanks for having a look!
I'm skipping the lines that lead to warning. If no one has any other comments I'll rebase

@yaugenst-flex
Copy link
Collaborator

Yeah I'd say that's fine for now. We should probably revisit the doctests at some point though, I've opened an issue (#1800)

@momchil-flex momchil-flex merged commit 8c092de into pre/2.8 Jul 5, 2024
16 checks passed
@momchil-flex momchil-flex deleted the marc/docstring-fix branch July 5, 2024 08:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.8 will go into version 2.8.*
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants