Skip to content

WIP: added hxsurf addition#482

Merged
jefferis merged 7 commits intonatverse:masterfrom
dokato:hxsurf-add
Sep 11, 2021
Merged

WIP: added hxsurf addition#482
jefferis merged 7 commits intonatverse:masterfrom
dokato:hxsurf-add

Conversation

@dokato
Copy link
Copy Markdown
Contributor

@dokato dokato commented Sep 1, 2021

I needed an easy way to concatenate 2 hxsurfs, I did not find any better way to do it, so I implemented this simple method. For example:

h1 = as.hxsurf(icosahedron3d(), 'a')
h2 = as.hxsurf(tetrahedron3d(), 'b')
h3=h1+h2
plot3d(h3)

Screenshot 2021-09-01 at 15 20 38

The problem is that right now hxsurf inherits operations from dotprops, which makes sense for most but not all of them.

Ops.hxsurf <- Ops.dotprops

I'm not sure how to point the package to what is the desired method to call in that case.

@codecov
Copy link
Copy Markdown

codecov bot commented Sep 1, 2021 •
edited
Loading

Codecov Report

Merging #482 (a30153e) into master (b8d420c) will increase coverage by 0.04%.
The diff coverage is 93.33%.

❗ Current head a30153e differs from pull request most recent head 2cac57d. Consider uploading reports for the commit 2cac57d to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #482      +/-   ##
==========================================
+ Coverage   76.80%   76.84%   +0.04%     
==========================================
  Files          47       47              
  Lines        5863     5878      +15     
==========================================
+ Hits         4503     4517      +14     
- Misses       1360     1361       +1
Impacted Files Coverage Δ
R/hxsurf.R 91.15% <93.33%> (+0.10%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b8d420c...2cac57d. Read the comment docs.

@jefferis
Copy link
Copy Markdown
Collaborator

jefferis commented Sep 6, 2021 •
edited
Loading

Thanks a lot for this @dokato. I think this is where S3 methods are not so good as really the method to choose here depends on both arguments i.e. whether to do arithmetic or join things together. I would actually sidestep this by using c i.e. c.hxsurf or merge rather than + for this functionality.

jefferis
jefferis reviewed Sep 7, 2021
View reviewed changes
jefferis
jefferis reviewed Sep 7, 2021
View reviewed changes
jefferis
jefferis approved these changes Sep 7, 2021
View reviewed changes
Copy link
Copy Markdown
Collaborator

@jefferis jefferis left a comment

Choose a reason for hiding this comment

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

Looks great. Thank you!

@jefferis jefferis merged commit 3f69a6c into natverse:master Sep 11, 2021
@dokato dokato deleted the hxsurf-add branch April 21, 2022 14:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jefferis jefferis jefferis approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@dokato @jefferis