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

Merge wh-questions library in its current state #502

Merged
merged 72 commits into from
Sep 1, 2020
Merged

Merge wh-questions library in its current state #502

merged 72 commits into from
Sep 1, 2020

Conversation

olzama
Copy link
Collaborator

@olzama olzama commented Aug 27, 2020

OK....

I would like to merge the wh-library in now. This PR is huge, and I am not sure anybody will want to review it (which is fine).

Now that we started using Github, I suggested to Emily that the new libraries are no longer developed in the previous fashion, when the person would work on their library for months if not years. That results in overwhelmingly difficult merges and makes careful review virtually impossible.

Perhaps in the future, people work on short self-contained branches, one (sub)phenomenon at a time, whichever the minimal work item could be defined without breaking tests, and then comfortably merges into the trunk, not worrying about the website and the actual release of the library in full. The trunk can be separate from the website.

Now, this PR contains lots of various changes, most having to do with wh-questions but some having to do with things like ICONS updates and making verbs SPR-underspecified (and lots of other things SPEC-empty).

The regression tests have been updated mostly with respect to ICONS, as I fixed some of formerly broken appends and the ICONS started showing up. Conversely, I fixed a type in matrix.tdl which contributed ICONS which did not yet make sense, so, I got rid of those underspecified ICONS in some 50 tests.

There is (at least) one regression which I will not fix now (because I do not know how) but will file an issue: it is item 197 in the Russian wh test (wh-rus-dev). There is a new ICONS there which I don't want, but similar things have always been in other wh-tests. Not sure why this one is new. Must be a formerly broken append as well. Now, I don't want any of them, but they will stay for now.

There is still plenty of work to do. For example, I need to simplify the WH feature to only contain an OR (it currently also has an unused AND). I will file an issue.

The information structure in wh-questions currently is not right. Again, will file an issue and hopefully will address it some time soon.

But I want to merge this in and start working on short branches instead.

@olzama
Copy link
Collaborator Author

olzama commented Aug 31, 2020

My primary concern is the deletion of tests. Many of the deleted tests are clausal complement tests, so I'm assuming there's some knowledge there, but there's a bunch of deleted adnominal possession tests and some other ones (e.g. neg1b-neg2fd), too? Should these deleted test just be marked skipped if they're not working? But I think they are working on trunk?

See #499 for these. If they used to pass in the trunk, that would be by mistake (empty gold or something like that). All these tests either are mostly empty or are my own tests which I know I wanted to delete. Emily and I agreed there is no reason to keep them around in the hopes that someone fixes them one day; too many SKIPPED tests is also bad and should be reserved for conscious decisions. If a test seems to be missing most of its content, there is probably little reason to keep it around.

@olzama
Copy link
Collaborator Author

olzama commented Aug 31, 2020

My secondary concern is the new globals. There are already (true) globals in the code that could be removed, but adding new ones seems mediocre. I understand there may be a philosophical divide here, but I'm sort of confused about their purpose? It seems you're trying to avoid adding the same feature twice, but that shouldn't be a big deal? Also, other libraries already do this (going back to SFD days) by storing choices in the choices file. Something like this seems preferable. In the choices refactor work I've done, I've thought that it would be handy to have a copy of the user's input choices and an "enriched" choices, and maybe this would accomplish what you're going after? As I mentioned in the comment though, this can be addressed later.

Yeah, this is by all means unfinished (and yes, a philosophical divide may emerge in a discussion like this, I suppose!).

What I was after is not having to compute a value for a feature more than once. There are decisions which need to be made based on a combination of choices, for example: are there question particles which exhibit diverse behavior with respect to what type of questions they occur with? Having to compute that twice (or more than once) is not ideal. Now, what I strongly believe needs to be avoided is duplicate code (because typos and bugs); but what could be done instead of the "globals" is a function that gets called in all places. Then the value would be computed more than once but at least by the same place in the code. Let's open an issue about this.

Yes, "enriched choices" sounds like something I am after here, perhaps.

@olzama
Copy link
Collaborator Author

olzama commented Aug 31, 2020

Alright, many thanks, @dantiston for the review. I addressed some things he pointed out and opened a couple issues for the remaining things which I do not want to address now. I will now finally merge this PR in and hopefully we will never see a PR of this size here ever again!! :)

@olzama olzama requested a review from dantiston August 31, 2020 20:19
@goodmami
Copy link
Member

goodmami commented Sep 1, 2020

Thanks @dantiston for doing the review. I added some comments to issues you've raised. I agree that we should not introduce new globals and instead should work towards removing any that exist.

@olzama olzama linked an issue Sep 1, 2020 that may be closed by this pull request
@goodmami
Copy link
Member

goodmami commented Sep 1, 2020

@olzama you've linked #506 to this pull request, meaning that it will be closed when this is merged. Is that what you want?

@olzama
Copy link
Collaborator Author

olzama commented Sep 1, 2020

@olzama you've linked #506 to this pull request, meaning that it will be closed when this is merged. Is that what you want?

Yes; I addressed that already!

@goodmami
Copy link
Member

goodmami commented Sep 1, 2020

Yes; I addressed that already!

Oh, sorry, I thought it was a new issue created as a response to this PR review.

Copy link
Contributor

@dantiston dantiston left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @olzama for making those changes! I think it looks good!

I think there are still some outstanding issues, such as renaming constants, changing the example type to a comment, some other pending clean-ups. But, it seems like there is consensus about what needs to change and how to change it, so I'll go ahead and accept! Please finish the clean-ups before merging.

@olzama
Copy link
Collaborator Author

olzama commented Sep 1, 2020

Yes; I addressed that already!

Oh, sorry, I thought it was a new issue created as a response to this PR review.

It was actually; I created it thinking I won't address it just yet but then went ahead and did. Sorry for the confusion.

@olzama
Copy link
Collaborator Author

olzama commented Sep 1, 2020

Thanks a lot, @dantiston and @goodmami!
Interesting discussion :).

@olzama olzama merged commit 7c4832a into trunk Sep 1, 2020
@olzama
Copy link
Collaborator Author

olzama commented Sep 2, 2020

Re style policies: given that we seem to fundamentally disagree, maybe we should have a zoom meeting some time, instead of discussing it further via the forum? I think the forum both (i) can come across not the way the person means, and (ii) actually brings out additional aggressiveness/defensiveness in people sometimes. So maybe a meeting would be preferable (for communication purposes rather than for making any specific decisions, if for no other reason than simply because it is not very clear how any decision could technically be made :) ).

@goodmami
Copy link
Member

goodmami commented Sep 2, 2020

Sounds good. Let's set it up by email.

@olzama olzama deleted the merge-wh branch September 3, 2020 00:26
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.

div_particles and globals.py
3 participants