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

[WIP] Drop prototype features to focus on core functionality #492

Merged
merged 1 commit into from
Jan 27, 2014

Conversation

johnmyleswhite
Copy link
Contributor

This removes several non-core pieces of functionality that we don't have the resources to maintain:

  • Expression-based indexing
  • The DataStream protocol
  • The NamedArray type

Right now I haven't updated the tests to indicate the removal of functionality.

@nalimilan
Copy link
Member

Removing NamedArray sounds like a good idea if we want to use this name for more general "named" or "associative" arrays.

Regarding with() and friends, why not move them to prototypes rather than removing them? I agree that's not a big difference, but I wonder whether you think they should be readded at some point, or whether a better alternative should be found.

@tshort
Copy link
Contributor

tshort commented Jan 22, 2014

I think the plan is to try out different approaches to with() and friends to see if we can overcome the problems with the existing implementations. This could be something like LINQ or a @select macro.

@johnmyleswhite
Copy link
Contributor Author

I didn't move with to prototypes just to save programming effort. That's easy enough to undo. But I'm not totally sold on with (or any of the delayed evaluation tools) in the long term. It has two properties I strongly dislike:

  • It duplicates functionality we already have. It's like syntactic sugar at the interface level, since you can always express a with operation by fully qualifying column names.
  • It provides an implementation of core functionality that is non-trivially worse than the equivalent core functionality, because you need to understand subtle details about how Julia's expressions and scopes work to use with correctly. Saving a few characters doesn't seem to justify the very substantial conceptual complexity that with brings with it.

@nalimilan
Copy link
Member

Yeah, with() is clearly syntactic sugar, but whether it is useful syntactic sugar or not depends on whether we find a better solution as @tshort said. I agree that for now it's OK to remove it.

@johnmyleswhite
Copy link
Contributor Author

This is much closer to being done. The one trouble is that some of the code in formula.jl depends on with, so I need to fix that.

I changed the way tests are run to be more informative. If you type julia test/runtests.jl -f you'll get the old behavior where the first error is fatal. Otherwise you get a summary of each test's success/failure.

@johnmyleswhite
Copy link
Contributor Author

I think this is ready for review now.

@johnmyleswhite
Copy link
Contributor Author

I decided to not try to move with and friends to prototypes because I don't currently see a strategy for making them work well. For select, I opened another issue where I proposed using placeholders to get some of the effects of delayed evaluation. If people are ok with doing the same thing for with, I think we could make it work.

@johnmyleswhite
Copy link
Contributor Author

Bump. I'd really like to merge this soon.

@tshort
Copy link
Contributor

tshort commented Jan 26, 2014

I like your deletions and reorganizations.

@johnmyleswhite
Copy link
Contributor Author

I unfortunately can't merge this yet because the current formula definition depends on with. I'm working to fix it now.

- Expression-based indexing
- DataStream
- NamedArray
@johnmyleswhite
Copy link
Contributor Author

I made the minimum fix required to get simple GLM functionality working again. I'll do my best to restore everything else by the end of the night, but I'm going to merge this now to get it over with.

johnmyleswhite added a commit that referenced this pull request Jan 27, 2014
[WIP] Drop prototype features to focus on core functionality
@johnmyleswhite johnmyleswhite merged commit 9321f9a into master Jan 27, 2014
@garborg garborg deleted the jmw/purge branch November 25, 2014 23:11
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.

3 participants