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

Deprecate flipud() and fliplr() in favor of flipdim() #10446

Merged
merged 1 commit into from
Mar 9, 2015

Conversation

nalimilan
Copy link
Member

We do not provide other convenience functions for any other
operation, like size(), and these names are obscure for
non-MATLAB users.

I found only one use of flipud in base, and none of fliplr.

BTW, I discovered something weird: if I call flipud first in the Julia session, calls to fliplr do not trigger a deprecation warning; if I call the latter first, the reverse is true. Is that expected?

@simonster
Copy link
Member

Seems reasonable to me. We should also delete the original definitions and the docs. Not sure what's going on with the deprecations, but they look right.

@ViralBShah
Copy link
Member

+1

@ivarne
Copy link
Member

ivarne commented Mar 9, 2015

Depreciation messages depends on backtraces, and if they are unreliable, you'll get unpredictable results.

@tknopp
Copy link
Contributor

tknopp commented Mar 9, 2015

-1 from me. But this might be as I am used to these functions and they are really convenient in plotting code when the image is mirrored upside down.

@nalimilan
Copy link
Member Author

@simonster I thought we would keep the definitions for one cycle, and only remove them after that? Isn't that our deprecation policy?

@tknopp
Copy link
Contributor

tknopp commented Mar 9, 2015

@nalimilan: The @deprecate macro will define them, so everything will work if you remove the original definitions.

@nalimilan
Copy link
Member Author

OK, I've updated the PR to remove the old definitions and the docs.

@ivarne
Copy link
Member

ivarne commented Mar 9, 2015

I don't know these functions, but if this is to be merged. we would also need a mention in NEWS.md

@tknopp
Copy link
Contributor

tknopp commented Mar 9, 2015

For me these are pretty core functions that operate on arrays and IMHO before cleaning up in this corner there are different areas that should be first removed from base. The sparse matrices for instance.

@johnmyleswhite
Copy link
Member

+1 to removing these

@simonster
Copy link
Member

@tknopp The functionality is still there and will only take 4 more characters to get. I doubt these aliases will be missed by anyone but MATLAB users. If we're going to deprecate these, I don't see why waiting would be advantageous, and moving sparse matrices out of Base seems completely orthogonal.

@nalimilan
Copy link
Member Author

@tknopp I understand, I'm also used to nrow and ncol (from R) instead of size(A, 1) and size(A, 2). But in the end I think it makes the language simpler and more consistent. It could be interesting to create a MATLAB.jl package which would provide compatibility functions, giving indications about the idiomatic Julia replacements either in the help or via deprecation warnings. It would help people get more familiar with Julia and ease porting code.

I've updated the PR with a NEWS entry.

@JeffBezanson
Copy link
Member

+1

Also needs to be removed from exports.jl. @deprecate takes care of exporting.

@IainNZ
Copy link
Member

IainNZ commented Mar 9, 2015

@nalimilan funny enough, that package exists: https://github.com/MatlabCompat/MatlabCompat.jl by @ayakimovich

We do not provide other convenience functions for any other
operation, like size(), and these names are obscure for
non-MATLAB users.
@nalimilan
Copy link
Member Author

Updated PR with removed exports.

@IainNZ Interesting!

JeffBezanson added a commit that referenced this pull request Mar 9, 2015
Deprecate flipud() and fliplr() in favor of flipdim()
@JeffBezanson JeffBezanson merged commit 7306091 into master Mar 9, 2015
@nalimilan nalimilan deleted the nalimilan/flip branch March 9, 2015 22:06
@tknopp
Copy link
Contributor

tknopp commented Mar 9, 2015

@simonster It seems I have not articulated my concern well enough. You are absolutely right that the sparse matrix removal is orthogonal to this one. And while I am absolutely no fan of maintaining Matlab compatibility where their API is confusing, one cannot say that Julia takes in general an entirely different approach to API naming. In this case I simply cannot see what the issue with the function names was.

@tkelman
Copy link
Contributor

tkelman commented Mar 10, 2015

Part of the goal of slimming down base is also slimming down the clutter in the default global namespace. As we've seen elsewhere, many of the obstacles to removing code from Base are related to naming conflicts. Multiple versions of different modules, either from different versions of Base or packages, both trying to export incompatible definitions of the same methods or types.

We could have put these two methods behind a non-default import list. I think that will be the first step before we try removing anything else that existing packages depend on. For example right now I wouldn't terribly mind unexporting the sparse matrix functionality, one more import/using statement isn't the end of the world, but completely removing it from base would break all of JuliaOpt.

@ViralBShah
Copy link
Member

This has nothing to do with trimming Base, but API cleanup.

To @tknopp 's point, the flipdim is slightly less readable than flipud/fliplr. It would be nice if we could allow flipdim(A, Rows) for example. Perhaps we can have a dimension type that can be passed to various array functions, rather than only a number for the dimension. This could be more readable.

@ayakimovich
Copy link

@IainNZ thanks for the reference
@nalimilan that's true, just few moth ago I started the project aimed at providing matlab code compatibility to Julia. The incentive behind it is to attract more attention to Julia in the heterogeneous environment of technical computing and academia. I completely understand concerns that some people would rather see Julia free of the matlab mess, but my incentives are rather pragmatic and hopefully the package can serve to interest people from the environment I work in more in Julia.

Now I currently work on the project together with a colleague and we address more the image analysis part. But since I have set up the repository as an organization there is an easy way to get into the project if you are interested. The more the merrier.

@nalimilan
Copy link
Member Author

@ayakimovich I've never really used Matlab myself, but I find it useful to provide a compatibility package for people who want to make the transition to Julia.

@ViralBShah
Copy link
Member

I certainly think a compatibility package is useful - especially when porting codes and such.

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.

10 participants