-
Notifications
You must be signed in to change notification settings - Fork 0
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
Recursive print algorithm #24
Comments
I ended up having to edit a lot of stuff across many files to make it work, and I also worked with an old version of the files (locally), so it will take some time to push the changes unfortunately |
Hey that's great you got it working! Are you able to push a new branch? I made quite a lot of changes to get things packaged and easy to use, but the changes I made should be pretty easy to separate. We can discuss more later today. |
I might be able to get started on that—I'll get it done probably right before the meeting. Also, in case you saw this in the output above, I fixed the error for the verifiers of |
embedded counterfactuals (as in |
I gave it a try but am getting some repeated print statements (this was before the last push). I tried to figure out what the problem was but it wasn't clear to me. I included an extra print statement in
I can't test this since it is not running currently but that was the thought. Although I think we should try to start only working on branches given the current complexity of the project, it is also important to test that local changes work before pushing. But no worries about the last push. |
That's a good point, it is a good idea to start working on branches (sorry about the last push). I also noticed this and I think it is now fixed. |
No worries! And nice work with the fix! It's printing great. Feel free to ignore the branches I created, but also feel free to create and push new branches as needed. |
I got the printing working reasonably well. There may be some more minor changes I'll make at some point, but it reads pretty well right now. |
I think I've gotten a little clearer on the
I think this basically is how it works, but some minor changes. Not all the names match, but hopefully clear enough what each thing is. |
I changed I'm also a little confused about the |
Yeah, so suppose we have a Proposition object with When an extensional Proposition object is initialized, it gets by default its comparison world as the model's input world (the immutable one), and the alt worlds are calculated from that. However in the case of evaluating a counterfactual, the comparison world changes, in which case the This way of doing things should work but in editing it I am realizing that it is perhaps not the most intuitive and results in lots of unnecessary computations. I'm going to try to get rid of those two things in the next push. |
That helps! I there is something odd about having the Regarding a name for the fixed world, I think "designated_world" might be a good choice. I'll change |
Yeah, I think an alternative along those lines would be better—I think even better would be only to calculate the alt worlds when they're needed, because they're not even needed that often and in any case where we store all potentially relevant alt worlds we're doing a lot of unnecessary stuff |
Nice, calculating as we go sounds ideal. Maybe the |
I just pushed stuff to a new branch called |
Awesome! I'll check it out and let you merge the branch when you are happy with it. |
Just took a look and made one note. It's looking good. I made minor changes to |
Sounds good! Here is what
It's definitely clearer, but refactoring it like you were thinking may be nice. What do you think? If we refactor, I was thinking of putting the helper functions in |
I just pushed everything and merged. I think most of the things you did ended up being undone, so you may have to add them again (sorry!). I did a quick test run and it works, lmk if it doesn't on your end |
Hey just went through it and it's looks great! As for moving helper functions into |
I finished fixing up |
Sounds good! I'll start documenting things and in that process it'll become clear what would move where. I think that having another file called |
Awesome sounds great! I just pushed a patch for the disjunctive conclusions feature I added. Seems to be working well. I'm going to see if I can figure out how to write unite tests so that I can avoid testing all of the examples one by one before merging new features. |
You mentioned that there were some functions that might make sense to move to |
I looked through it and couldn't find anything. I remember saying that, but I think it was just to double check that nothing should be moved. It's cleaning up nicely! |
Ah gotcha. Also, as a general rule of thumb, I was thinking it'd be nice most of the methods for the ModelStructure class were printing methods, and everything the user would want to otherwise see could be accessible by the attributes or the Proposition class. Now, the user can find a proposition by inputting an infix sentence into the |
That sounds great! We're getting close! |
I've been going back through
This function gets called in |
I ran some tests and it can't see that
My hope is to remove all unnecessary elements, simplifying the code. |
I think this is resolved, or at least old and likely to get refactored significantly. |
Hey Ben, I've been working on the recursive print algorithm and I got it working (as in not crashing; not necessarily correct hahaha). Here's what the function prints for input
premises = ['\\neg A','(A \\boxright (B \\vee C))']
andconclusions = ['(A \\boxright B)','(A \\boxright C)']
:The text was updated successfully, but these errors were encountered: