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

Change sample dataset name. #3351

Merged
merged 2 commits into from
Jun 20, 2021

Conversation

brittonsmith
Copy link
Member

PR Summary

PR Checklist

Changes the name of the dataset to an actual existing sample dataset.

  • New features are documented, with docstrings and narrative docs
  • Adds a test for any bugs fixed. Adds tests for new features.

@matthewturk
Copy link
Member

Looks like this is a temporary failure, so I'm re-running the jobs.

2021-06-14T09:12:47.4206210Z Submodule 'answer-store' (https://github.com/yt-project/answer-store) registered for path 'answer-store'
2021-06-14T09:12:47.4386390Z Cloning into '/Users/runner/work/yt/yt/answer-store'...
2021-06-14T09:13:05.7479760Z ##[error]fatal: unable to access 'https://github.com/yt-project/answer-store/': LibreSSL SSL_read: SSL_ERROR_SYSCALL, errno 60

@Xarthisius
Copy link
Member

Since this PR is all about making me happy I need to stress that what irked me about #3305 is not the fact that submitted code was not working (cause errare humanum est), but the fact it received 3 positive reviews. Looks like this PR is heading towards the same state, which underlines the sad fact that nobody bothers to even glance at the docs that are built during the testing process. It's mind-boggling since the original PR touched only *.rst files.

I haven't looked closely yet but there are at least two things that immediately stand out:

  1. References to methods in doc/source/visualizing/plots.rst are erroneously using :ref: instead of :class: or :func: which prevents them from rendering properly (see link or image below):

bug1

  1. Additions to API Reference are missing completely, cause they're missing class in their signature. I think it should be:
diff --git a/doc/source/reference/api/api.rst b/doc/source/reference/api/api.rst
index 32bcfe64d..52a057bf7 100644
--- a/doc/source/reference/api/api.rst
+++ b/doc/source/reference/api/api.rst
@@ -531,11 +531,11 @@ Field Functions
 
    ~yt.fields.field_info_container.FieldInfoContainer.add_field
    ~yt.data_objects.static_output.Dataset.add_field
-   ~yt.data_objects.static_outputs.add_deposited_particle_field
-   ~yt.data_objects.static_outputs.add_mesh_sampling_particle_field
-   ~yt.data_objects.static_outputs.add_smoothed_particle_field
-   ~yt.data_objects.static_outputs.add_gradient_fields
-   ~yt.frontends.stream.data_structures.add_SPH_fields
+   ~yt.data_objects.static_outputs.Dataset.add_deposited_particle_field
+   ~yt.data_objects.static_outputs.Dataset.add_mesh_sampling_particle_field
+   ~yt.data_objects.static_outputs.Dataset.add_smoothed_particle_field
+   ~yt.data_objects.static_outputs.Dataset.add_gradient_fields
+   ~yt.frontends.stream.data_structures.StreamParticlesDataset.add_sph_fields
 
 Particle Filters
 ----------------

but I leave it to @brittonsmith to figure it out :)

@neutrinoceros
Copy link
Member

which underlines the sad fact that nobody bothers to even glance at the docs that are built during the testing process.

I ... didn't know that it was possible ? I'm already struggling to find the logs for any one job on Jenkins, it never crossed my mind that the resulting artifacts would deploy anywhere (even now it seems I can't find where that would be).

I reviewed this PR by checking that the new dataset name existed and already loadable with load_sample. Does it have to include other corrections to go in ?

@brittonsmith
Copy link
Member Author

@Xarthisius, really sorry about that! I wrongly assumed the docs build would not show as passed if there were errors or warnings. It's a good reminder to be checking the render of the docs on PRs that change them. It's been so long since I've been able to build them locally. If it's not currently that way, perhaps we should make it a goal to make the docs build warning-free so we can be testing explicitly.

@neutrinoceros
Copy link
Member

@brittonsmith amen to that :)

@neutrinoceros
Copy link
Member

@yt-fido test this please

1 similar comment
@neutrinoceros
Copy link
Member

@yt-fido test this please

@neutrinoceros neutrinoceros merged commit ff4937c into yt-project:main Jun 20, 2021
@brittonsmith brittonsmith deleted the makekacperhappy branch June 22, 2021 09:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants