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

Add Cylinder function #1451

Merged
merged 1 commit into from
Jun 27, 2021
Merged

Add Cylinder function #1451

merged 1 commit into from
Jun 27, 2021

Conversation

TiagoCavalcante
Copy link
Contributor

@TiagoCavalcante TiagoCavalcante commented Jun 25, 2021

Rebase of #1450.

Closes #1294.

@rocky rocky force-pushed the master branch 3 times, most recently from 9570fdd to 499f1bf Compare June 26, 2021 14:12
@TiagoCavalcante
Copy link
Contributor Author

@rocky could you review my changes? I put a FIXME here I don't know to transform the current data in a draw around this axis.

@rocky
Copy link
Member

rocky commented Jun 26, 2021

@rocky could you review my changes? I put a FIXME here I don't know to transform the current data in a draw around this axis.

Right now there isn't a transform function, but there should be one. In #1373 there is code that needs to be adapted here

@TiagoCavalcante
Copy link
Contributor Author

@rocky you think it's better we do that before we merge?

Also, what do you think about the odd number of coordinates handling?

@rocky
Copy link
Member

rocky commented Jun 26, 2021

Here is a different idea for how to approach this and at the same time go in the direction we want to go in....

Write a to_json() routine for this. But actually, I'd start out with sphere, since that might be easier and here we want to focus on the JSON aspect.

Then we should start a graphics-backend module. (In the past, it was erroneously called a pymathics module, but I realize this was wrong. This is a pure module of any kind that takes JSON with our simple graphics API and converts it into threejs. You could start out with graphics3d.js and/or mathics.js from the Django repository and turn that into a nodejs module (or however you want to package it).

@rocky
Copy link
Member

rocky commented Jun 26, 2021

@rocky you think it's better we do that before we merge?

Also, what do you think about the odd number of coordinates handling?

Release 3.0.0 of Mathics core and Django are currently out. Look on PyPI or in "releases" of this project.

@rocky
Copy link
Member

rocky commented Jun 26, 2021

Release 3.0.0 of Mathics core and Django are currently out. Look on PyPI or in "releases" of this project.

I am still working on mathics omnibus though.

@TiagoCavalcante
Copy link
Contributor Author

TiagoCavalcante commented Jun 26, 2021

@rocky you think it's better we do that before we merge?
Also, what do you think about the odd number of coordinates handling?

Release 3.0.0 of Mathics core and Django are currently out. Look on PyPI or in "releases" of this project.

@rocky I didn't find the information I need here.

Write a to_json() routine for this. But actually, I'd start out with sphere, since that might be easier and here we want to focus on the JSON aspect.

Ok, I guess it should be in another PR, isn't it?

Then we should start a graphics-backend module. (In the past, it was erroneously called a pymathics module, but I realize this was wrong. This is a pure module of any kind that takes JSON with our simple graphics API and converts it into threejs. You could start out with graphics3d.js and/or mathics.js from the Django repository and turn that into a nodejs module (or however you want to package it).

@rocky what are we going to use to get the package JavaScript files, Git submodules?

@rocky
Copy link
Member

rocky commented Jun 26, 2021

Release 3.0.0 of Mathics core and Django are currently out. Look on PyPI or in "releases" of this project.

@rocky I didn't find the information I need here.

I don't understand what information you need.

All I was saying is that there are new releases: https://pypi.org/project/Mathics3/ (pypi) and https://github.com/mathics/Mathics/releases/tag/3.0.0

What

Write a to_json() routine for this. But actually, I'd start out with sphere, since that might be easier and here we want to focus on the JSON aspect.

Ok, I guess it should be in another PR, isn't it?

Yes, all of this is a new PR. New approach, new PR. And let's try to start simple to work up to cylinders which add this additional wrinkle.

@TiagoCavalcante
Copy link
Contributor Author

TiagoCavalcante commented Jun 26, 2021

I don't understand what information you need.

Sorry for the confusion.

The information I was needing was what we should do when the user does something like:

Cylinder[{{0,0,0}}] (* it should raise an error because the user didn't provide an end point, just the start one *)

But you already answered here.

@TiagoCavalcante
Copy link
Contributor Author

I left a FIXME saying we need to do the following transformation_vector * (radius, radius, radius) instead of (1, 1, 1) * (radius, radius, radius). The current version does the plot range be bigger than it should.

As we are going to do the transformation part in other PR I guess this should be merged before that.

Also, the following is the behavior of an odd number of points:
image

Copy link
Member

@rocky rocky left a comment

Choose a reason for hiding this comment

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

LGTM - I wonder why we get the spurious black style wars.

Did you run black . ?

@rocky rocky merged commit c8af25a into mathics:master Jun 27, 2021
rocky added a commit that referenced this pull request Jun 27, 2021
@TiagoCavalcante TiagoCavalcante deleted the add-cylinder-function branch June 27, 2021 18:08
@TiagoCavalcante TiagoCavalcante restored the add-cylinder-function branch June 27, 2021 18:08
@TiagoCavalcante
Copy link
Contributor Author

@rocky no, I'm doing that (I've forgotten).

@rocky
Copy link
Member

rocky commented Jun 27, 2021

I suspect then it is the black version that is used. What does black --version say?

@TiagoCavalcante
Copy link
Contributor Author

TiagoCavalcante commented Jun 27, 2021

@rocky I don't have git pre-commit installed, I guess it's that.

My black version is 21.6b0.

Also, I already pushed a formatted version to this branch.

@rocky
Copy link
Member

rocky commented Jun 27, 2021

Ok - Let's set everything to 21.6b0.

I will add a check to the workflows too.

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.

2 participants