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 additional output type & skip empty optional arguments #104

Merged
merged 2 commits into from
Apr 13, 2023
Merged

Conversation

akash-akya
Copy link
Owner

@akash-akya akash-akya commented Apr 13, 2023

  • Change type of additional output values to map from keyword list
  • Skip empty optional argument from function definition when the operation does not have any

Hi @kipcole9! I am making few breaking changes, if possible can you check if this breaks Image?
Btw, I also made few other changes in the master - #103. Notably, added limited math operators similar to Image :)

@kipcole9
Copy link
Contributor

@akash-akya, I tried image with vix master branch and all tests pass without change. So far so good! I will very happily ditch my math operators in favour of yours - but I haven't tested that yet.

@akash-akya
Copy link
Owner Author

Thanks for quick check!

I will very happily ditch my math operators in favour of yours - but I haven't tested that yet.

👍🏼. Makes sense to just wait a bit more for it to be stable. I am thinking if we need something like Nx.Defn, but not finding strong reason to add that complexity yet.

@akash-akya akash-akya merged commit ad6acd7 into master Apr 13, 2023
@akash-akya akash-akya deleted the dev branch April 13, 2023 18:10
@kipcole9
Copy link
Contributor

TLDR; Some small incompatibilities I can work around.

I spoke too soon because I tested on master before you merge. Some small issue I can easily work around.

  • Operation.find_trim/2 now doesn't return flags as the last element in the return tuple (easy fix for me, no issue)
  • Vix.Vips.MutableOperation.draw_line/7 is now Vix.Vips.MutableOperation.draw_line/6 which should be easy to work around
  • Operation.profile/1 now doesn't return flags in the return tuple - also easily worked around in my code.

There is one other failing tests but the cause is not immediately clear. It is coming from Image.skew_angle/1 meaning its either Operation.fft or Operation.project that has a different return type I have to investigate.

@akash-akya
Copy link
Owner Author

I see. Let me know if you need any help with that, I can create a PR.

There is one other failing tests but the cause is not immediately clear. It is coming from Image.skew_angle/1 meaning its either Operation.fft or Operation.project that has a different return type I have to investigate.

It should be project, same as point one. Previously we returned empty list as last value, now we don't

@kipcole9
Copy link
Contributor

All good Akash, they are easy fixes and a cleaner API so happy to push another Image release when you ship yours.

Do you have a view on when you please to release this version of Vix?

@akash-akya
Copy link
Owner Author

Well then nothing is pending from my side, I'll create a release now.

@kipcole9
Copy link
Contributor

Sounds good, I'll get my lib back in shape after you publish and do the same. Glad you're still motivate by working on vix - its an amazing library and, if I can say, beautifully done.

@akash-akya
Copy link
Owner Author

akash-akya commented Apr 14, 2023

Thanks! Yes, still have a lot of motivation, only wish I had more time to spare, anw I'll complain about time some other day.

Few things I have on my mind in no order:

I am open to some other important changes as well if you have something on mind.

@akash-akya
Copy link
Owner Author

@kipcole9 v0.17.0 is released 🎉

@kipcole9
Copy link
Contributor

I'm very interested in where to you get to with those "roadmap" items - very interesting!

image 0.29.0 is published now with support for vix 0.17.0.

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