Skip to content

Conversation

odow
Copy link
Member

@odow odow commented Jul 26, 2021

These functions were not called anywhere. We should remove unused things. People can just use .value.

@odow odow added the breaking label Jul 26, 2021
@blegat
Copy link
Member

blegat commented Jul 26, 2021

It can be quite useful for solver wrappers to get the vector of index values from the vector of indices using broadcast. No solver use that ?

@odow
Copy link
Member Author

odow commented Jul 26, 2021

If solvers need it they can define it themselves. There's also getfield.(x, :value). When in doubt, I vote remove. We want to minimize the surface area of MOI if at all possible.

@blegat
Copy link
Member

blegat commented Jul 26, 2021

It would be more readable if solvers use all a same documented way for doing so getfield.(x, :value) seems a bit hacky.

We want to minimize the surface area of MOI if at all possible.

I agree but it does not seem that we would need to ever break this function.

OTOH, it's true that we may not want to have two ways to do the same thing so it all depends whether we would consider getfield.(x, :value) to be a recommended way or not.

@blegat blegat requested a review from mlubin July 26, 2021 10:11
@odow
Copy link
Member Author

odow commented Jul 26, 2021

It can't be that useful, because it isn't used anywhere in the whole of MOI. (Including in the tests...)

@odow
Copy link
Member Author

odow commented Jul 26, 2021

I have the majority of solvers dev'd to my machine

image

@odow odow added this to the v0.10 milestone Jul 27, 2021
@odow
Copy link
Member Author

odow commented Aug 3, 2021

Bump.

@odow
Copy link
Member Author

odow commented Aug 4, 2021

@Azzaare
Copy link

Azzaare commented Aug 4, 2021

I am on my phone atm so it will difficult to make the changes, but I don't mind updating the code to not use this function.

If one of you has a suggestion, I will update once I'm back home.

EDIT: I can change it to the get_field method above, but it does feel a bit like a hack haha. As long as it works, I don't mind.

@odow
Copy link
Member Author

odow commented Aug 4, 2021

You never use broadcast. You only use index_value(x). So it's just x.value instead. or you can define index_value locally.

I think it's safe to standardize the API on .value, so we shouldn't have two ways of doing this.

@odow
Copy link
Member Author

odow commented Aug 4, 2021

I found where it used to be used:


thanks to the JuliaHub search engine:
https://juliahub.com/ui/Search?q=index_value&type=code&w=true

@Azzaare
Copy link

Azzaare commented Aug 4, 2021

Since it is a breaking change, I suppose you will tag a new minor version.

In that case, you can merge this PR independently of CBLS.jl and I will make the change at the same I change the compat entry in the Project.toml

If for some reason the version will still be 0.9.x, let me know, and I will update before MOI.

@odow
Copy link
Member Author

odow commented Aug 4, 2021

I suppose you will tag a new minor version.

Yes. We're well into breaking change territory. The next release will be 0.10 and require lots of changes.

@odow odow merged commit 4b68503 into master Aug 4, 2021
@odow odow deleted the od/rm_index_value branch August 4, 2021 20:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Development

Successfully merging this pull request may close these issues.

3 participants