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] rename [batch_]psi methods to [batch_]joint_features #33

Closed
wants to merge 1 commit into from

Conversation

larsmans
Copy link
Contributor

I haven't run the tests yet; this affects the entire codebase and the full test suite is so slow on my box that I'm leaving it to Travis.

What to do with size_psi? Rename it joint_size?

@amueller
Copy link
Member

or joint_feature_size. It is used quite rarely I think.
What do the other think about this rename? @vene @fgregg @zaxtax ?

@amueller
Copy link
Member

btw @larsmans you can speed up all tests by a factor of ~10 by setting the default inference to ad3 in crf.py (provided you have it installed).

@larsmans
Copy link
Contributor Author

Haven't tried to install that yet, but I will if I find enough time to play with this stuff longer :)

@amueller
Copy link
Member

it should be in a state where you can install it via pip using the repo from the requirements file. if travis can do it, I'm sure you can do it, too ;)

@amueller
Copy link
Member

ok, another try pushed to master ;)

@zaxtax
Copy link
Member

zaxtax commented Jul 18, 2013

I'm ok with the rename.

@fgregg
Copy link
Contributor

fgregg commented Jul 18, 2013

I don't have a problem with it, but I also don't think it's any clearer.
Could you add some description of what join_features represent in say, the
base.py docstring.

On Thu, Jul 18, 2013 at 8:57 AM, zaxtax notifications@github.com wrote:

I'm ok with the rename.


Reply to this email directly or view it on GitHubhttps://github.com//pull/33#issuecomment-21184908
.

773.888.2718
2231 N. Monticello Ave
Chicago, IL 60647

@amueller
Copy link
Member

Ok, I agree that this is an improvement after I noticed the different uses if psi in graphical models and structured prediction. For someone reading Bishop, the name \psi is indeed quite confusing.

@amueller
Copy link
Member

unfortunately my realization came too late and I I have to deprecate ^^

@larsmans
Copy link
Contributor Author

Having read some more, I found out that in NLP at least, this is usually called Φ (which I think makes sense since it's a feature function).

@amueller
Copy link
Member

Yeah, I will also call it \phi in my thesis now - it is also called that in the Nowozin Lampert book ;)
Though joint_feature is surely a better function name than replacing one greek letter with another.

@amueller
Copy link
Member

Done in 354ea19.

@amueller amueller closed this Feb 23, 2014
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.

None yet

4 participants