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

Documentation for walk_volume #2449

Merged
merged 7 commits into from
Feb 11, 2020

Conversation

cgyurgyik
Copy link
Contributor

@cgyurgyik cgyurgyik commented Feb 10, 2020

Documentation for yt's grid traversal function walk_volume.

PR Summary

This ensures future maintainers and users are provided a basic understanding of walk_volume. Since it is such a large function, future steps may incorporate breaking the function up into two pieces, initialization and traversal.

PR Checklist

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

@welcome
Copy link

welcome bot commented Feb 10, 2020

Hi! Welcome, and thanks for opening this pull request. We have some guidelines for new pull requests, and soon you'll hear back about the results of our tests and continuous integration checks. Thank you for your contribution!

@cgyurgyik cgyurgyik marked this pull request as ready for review February 10, 2020 21:52
@ngoldbaum
Copy link
Member

I don't have a string opinion about this but I think it would be slightly more idiomatic to include add this documentation as a docstring on walk_volume rather than as a comment as you have it now.

ping @matthewturk who originally wrote this code and would hopefully be able to check the content of the new docstring.

@matthewturk
Copy link
Member

Content looks good, and I'm not sure a docstring makes much of a difference since it's on a cdef function without a python frontend. LGTM!

@cgyurgyik
Copy link
Contributor Author

Getting the following error from AppVeyor, though my code only has documentation:

GenericImage_UniformGridData_all_profile_dict_keys([('gas', 'cell_mass')])_18_None_None ... ok
GenericImage_UniformGridData_all_profile_odict_keys([('gas', 'density'), ('gas', 'dark_matter_density')])_None_None ... ok
yt.visualization.tests.test_profile_plots.test_set_units ... ok
yt.visualization.tests.test_profile_plots.test_set_labels ... ok
yt.visualization.tests.test_profile_plots.test_create_from_dataset ... ok
yt.visualization.tests.test_profile_plots.test_phaseplot_set_log ... ok
yt.visualization.tests.test_profile_plots.test_phaseplot_showhide_colorbar_axes ... ok
Tests functionality of off_axis_projection and write_projection. ... ok
create a toy dataset that puts a sphere at (0,0,0), a single cube ... ok
yt.tests.test_funcs.test_validate_axis ... ok
yt.tests.test_funcs.test_validate_center ... ok
yt.tests.test_testing.test_requires_backend ... ok
======================================================================
ERROR: yt.units.tests.test_ytarray.test_ufuncs
----------------------------------------------------------------------
Traceback (most recent call last):
  File "C:\Miniconda36-x64\envs\test\lib\site-packages\nose\case.py", line 197, in runTest
    self.test(*self.arg)
  File "C:\projects\yt\yt\units\tests\test_ytarray.py", line 878, in test_ufuncs
    unary_ufunc_comparison(ufunc, YTArray([.3, .4, .5], 'cm'))
  File "C:\projects\yt\yt\units\tests\test_ytarray.py", line 809, in unary_ufunc_comparison
    ret1, ret2 = ufunc(a)
  File "C:\projects\yt\yt\units\yt_array.py", line 1378, in __array_ufunc__
    out_arr = func(np.asarray(inp), out=out, **kwargs)
TypeError: 'out' must be a tuple of arrays
======================================================================
ERROR: test_make_colormap (yt.visualization.tests.test_color_maps.TestColorMaps)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "C:\Miniconda36-x64\envs\test\lib\site-packages\numpy\core\function_base.py", line 117, in linspace
    num = operator.index(num)
TypeError: 'numpy.float64' object cannot be interpreted as an integer
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
  File "C:\projects\yt\yt\visualization\tests\test_color_maps.py", line 50, in test_make_colormap
    name='french_flag', interpolate=False)
  File "C:\projects\yt\yt\visualization\color_maps.py", line 554, in make_colormap
    np.linspace(color[j], next_color[j], interval)
  File "<__array_function__ internals>", line 6, in linspace
  File "C:\Miniconda36-x64\envs\test\lib\site-packages\numpy\core\function_base.py", line 121, in linspace
    .format(type(num)))
TypeError: object of type <class 'numpy.float64'> cannot be safely interpreted as an integer.
----------------------------------------------------------------------
Ran 1430 tests in 1462.656s
FAILED (errors=2)
Command exited with code 1

@munkm
Copy link
Member

munkm commented Feb 11, 2020

Those are unrelated to your PR! They should be fixed in #2448.

@munkm
Copy link
Member

munkm commented Feb 11, 2020

Ok, that PR has been merged now. Would you mind merging master into your development branch just so we can check that everything passes?

Thanks so much for your work in this PR!

@ngoldbaum
Copy link
Member

I think we can just merge this? I don't really think we need to rerun the tests since we know that the failure is unrelated to this PR. Is that OK @munkm?

@munkm
Copy link
Member

munkm commented Feb 11, 2020

yep! Sounds good to me.

@ngoldbaum ngoldbaum merged commit c0d9254 into yt-project:master Feb 11, 2020
@welcome
Copy link

welcome bot commented Feb 11, 2020

Hooray! Congratulations on your first merged pull request! We hope we keep seeing you around! 🎆

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants