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

A Summary for References Who Cause Cyclic Importation Issues #1029

Closed
halonazhao opened this issue Oct 26, 2018 · 23 comments
Closed

A Summary for References Who Cause Cyclic Importation Issues #1029

halonazhao opened this issue Oct 26, 2018 · 23 comments

Comments

@halonazhao
Copy link
Collaborator

The importing conflits are as below:

Assumptions cannot import DataDefns;
Assumptions cannot import GenDefs;
Assumptions cannnot import IMods;
Assumptions cannnot import TMods;
DataDefns cannot import GenDefs;
DataDefns cannot import IMods;
TMods cannot import DataDefns;
TMods cannot import GenDefns;
TMods cannot import IMods;

Since even some chunks in the code can be reached, those chunks may be made references by their labels instead of referenced directly, so the code for cases doesn't really present the problem. So I think the good way to show all occurances of issues is to show in document. PDF shows all failing imports marked as Green.

Could you take a look see whether these information is sufficient? Could you get back to me once you see these? If I can get more instructions, I would still get time to make changes before the meeting. Thanks. @smiths @JacquesCarette

@smiths
Copy link
Collaborator

smiths commented Oct 29, 2018

Great summary @halonazhao. I believe I have found a relationship between TM, GD, DD and IM that will remove these problems. It will involve reclassifying some of the chunks we currently have in our case studies. I believe there will be a significant benefit though - the distinction between the types of information will become much more clear. I am going to run my idea by @JacquesCarette before we invest time into changing the examples.

@JacquesCarette
Copy link
Owner

Sounds good. Hopefully these thoughts are compatible with https://github.com/JacquesCarette/Drasil/wiki/General-Definitions which I wrote over the weekend. They are mainly about GD, but do also have some more general comments.

@smiths
Copy link
Collaborator

smiths commented Oct 29, 2018

@JacquesCarette, I don't see any inconsistencies between your wiki post and my thinking about the relation between GD, TM etc.

@halonazhao, I propose that we impose the following relationship between our chunks:

RelationsBetweenTM_GD_IM_DD_A.pdf

The theoretical models are refined to general definitions and the general definitions are refined to instanced models. The data definitions support the TMs, GDs and IMs. Assumptions support everything. The "may ref" arrows in the diagram show what is allowed to reference what. Starting from the bottom, we would want to import in the following order: Assumptions, Data Definitions, Theoretical Models, General Definitions, Instanced Models.

Applying this explicit relationship between chunks means that several of our examples will need to be changed. For instance, some of our general definitions should be data definitions and some of our data definitions should be general definitions.

I will go through the examples soon to highlight where I think the changes should be made. I believe with these changes, and your work on "back referencing" we should break all of the cycles.

@halonazhao
Copy link
Collaborator Author

Ok. I think the right order I should be working on is Assumption, DataDefinition, Theoretical Model, General Definition, Instance Model.

I may start with removing any manually inserted references in Assumption and DataDefn except they reference the same type as themselves. According to relations, these references can be generated automatically. And then others as Dr.@smiths may do some changes on them from manual version. Once Dr.Smith made the changes, I will keep updating Drasil to match. If there is any detail that I am not sure about, I would ask.

Is my plan ok so far? Or you want me to hold for more considerations? @smiths @JacquesCarette

@JacquesCarette
Copy link
Owner

Yes, that is a good plan.

However, whenever you remove such references which are not obvious 'back references' (i.e. could be computed by an algorithm that fills in RefBy or some such), please open an issue and assign it to @smiths and I. Roughly speaking, whenever there are references in the text body, we should look at it more deeply. Hopefully there will be few of these.

Please work on this incrementally. I don't mind having to review 30 tiny PRs.

@halonazhao
Copy link
Collaborator Author

Unfortunately, there is a problem here.
In GlassBR, standardValues is an assumption but it is not explictly referenced by any chunks showing here.

screen shot 2018-10-30 at 13 31 23

There should be many situations like this, so we may have lost the exactly right connections between assumptions and other chunks. These references are generated to match the manual version now and without any backwards referencing consistency check. Only if we can confirm that the manual version is exactly right, we can use these referencing information.

I guess the initial design here is that the references in assumptions indicating the usage of these assumptions. But after we thought the relations of chunks through, these assumptions should be better added to each chunk as information.

So there are two things I think we may need to do,
First is that we should confirm every connection from assumptions to chunks. Does 'chunk A' really build on 'assumption B'?
Second is that we may need to make a choice we want to show assumptions in the description of the chunk explictly or only to include these information in chunks as a new field without printing out.

@smiths
Copy link
Collaborator

smiths commented Oct 30, 2018

Yes, this is exactly what you should see, and the fix is what you have suggested. We have been lax in explicitly referencing assumptions from the chunks, but that is what we should have done. In some cases this information was in the manual version, but not carried through to the Drasil version (for instance, smiths/swhs#48).

The order that we are discussing now is the order that makes sense. The assumptions come up in the derivation of chunks. The assumption itself doesn't "care" where it is used. It is invoked as needed by different chunks, but it is not derived from the chunks.

To make this work, we will likely need to add a Notes field (or equivalent) to the data definitions. This is something we discussed doing previously and from my quick conversation with @JacquesCarette, this doesn't seem to be too daunting a task.

The assumptions should be added to the data definitions, IMs etc, and then the references attached to the assumptions back calculated.

You don't have to do all of these changes yourself. Others on the team are likely candidates for helping. We need to make any necessary changes to the manual version and to the Drasil version. We are going to need to be careful and methodical in the changes. For three of the case studies, I suggest the following helpers:

GlassBR: @vajihehm
SSP: @bmaclach
Game Physics: @oluowoj (I'm not sure Olu is a contributor on this repo, since I couldn't find her name with a simple search. (I looked at the Case Studies repo to get her github id.) @JacquesCarette, can you check whether Olu is a contributor on this repo?)

@halonazhao
Copy link
Collaborator Author

So far, the references in assumptions are moved to each chunk. And the DataDefinition references were added to other chunks. When I started doing the theory-model references, there are conflicts with data definition, which means there are DataDefinitions referencing the Theoretical Model and they should be addressed in the manual version first.

@smiths
Copy link
Collaborator

smiths commented Nov 7, 2018

@halonazhao, can you tell me which DDs are referencing TMs? I am confident that they can be rewritten to remove this problem, but I would like to have a look at them. Are the problems taken care of by issues:

smiths/caseStudies#203

smiths/caseStudies#202

These issues deal with Game Physics and GlassBR, respectively. I looked at SWHS, but I don't believe this problem crops up there.

Although there isn't an issue about it, @bmaclach and I are working on revising SSP. @bmaclach has a marked up physical copy of the SRS for SSP, but it is still a work in progress.

If these are the changes that are on your critical path, we'll have to think about how to best proceed. @vajihehm and Olu will eventually work on their changes in Drasil, but I'm not sure how long it will take for them to get to it. We might want to redistribute some of the work in updating the relevant parts of the case studies and porting this to Drasil. Hopefully we can sort this out during our meeting on Friday (Nov 9).

@halonazhao
Copy link
Collaborator Author

Ok. So I went through all the cases, it turned out there is only one conflict.
GamePhysics:
DD8 (Impulse for Collision response) references T4
I guess I really have "lucky" hand, the first one I opened is the one has the problem and the only one!

@smiths
Copy link
Collaborator

smiths commented Nov 9, 2018

@halonazhao, I am confused by where you see DD8 referencing T4. In your pdf at the beginning of this issue, you show DD8 referencing several GDs. There is no mention of T4. Are you using an updated version based on the changes from:

smiths/caseStudies@7508a15

If that is the case, I thought all of the GDs were turned into DDs, which I don't think would be a problem.

The easiest approach is to discuss this during our meeting later today.

@halonazhao
Copy link
Collaborator Author

halonazhao commented Nov 9, 2018

I found in https://github.com/smiths/caseStudies/blob/master/CaseStudies/gamephys/docs/SRS/GamePhysicsSRS.pdf.

So initially, the problem is to address the Cyclic Importation Issue. What I showed at beginning are only the difficulities from the code. But after we figured that the hireracy of those chunks is the actual problem. You sorted the hireracy out and posted https://github.com/JacquesCarette/Drasil/files/2527038/RelationsBetweenTM_GD_IM_DD_A.pdf

The problem got bigger than what appears in the code. So all the relations between chunks need review instead of only those ones with cyclic importation issue.

And according to your relation figure, DD can only ref itself or Assumption. @smiths

@smiths
Copy link
Collaborator

smiths commented Nov 9, 2018

@halonazhao, I agree that DD can only reference themselves and assumptions. Does it help that T4 (Chasles? theorem) should actually be a DD? I wrote that comment in the pdf file that you linked.

@halonazhao
Copy link
Collaborator Author

Yes, that definitely helps. Sorry that I didn't see the comment of T4. But I don't know should I go ahead change them in Drasil or wait until case study is updated? I will check whether there are more conflicts. I think I can work out a list first to see whether that would help us get all modifications done faster.

@smiths
Copy link
Collaborator

smiths commented Nov 9, 2018

The original plan was for Olu to update Drasil to make these changes. This might not be able to happen quickly enough for your schedule. We can discuss today.

@halonazhao
Copy link
Collaborator Author

Hi Dr. @smiths ,
I am sorry that I have to push on this issue again. As we made some progress on making the new ref2 type, and for DD, TM, IM, TM, we are able to apply new ref type on them. I was trying to modify the GlassBR to try to get rid of all the label referencing to use the new ref yesterday, but the 'manual' version seems not being updated yet. I saw @vajihehm made some progress on glassBr_review branch and you definitely need time to review all those changes. Just want to know would it be in a short time to do the merge. If not, I have a suggestion as below.

I realized that there are many major changes like some Chunks will be turned into another type of Chunk. So I think to migrate the changes from manual to Drasil will take some time too. So my suggestion, for now, is that I would get rid of the references from Drasil which violates your reference hierarchy. This would allow me starting doing the whole auto references-generation things so that you guys would have more time working on the manual version. The left references should be fairly sufficient for me to run some tests. In the meantime, I will migrate changes from manual to Drasil along the process once there are final changes for GlassBR. Is this plan ok for now? @JacquesCarette

@smiths
Copy link
Collaborator

smiths commented Nov 21, 2018

@halonazhao, I completed my review of the changes the @vajihehm made on the glass_revisions branch of the case studies repo (smiths/caseStudies#202 (comment)). @vajihehm has to still address some of my questions/comments, but not for the referencing. I have approved of the changes she has made there.

My preference would be to fix the referencing problem by updating Drasil to match the case study with respect to classification of DDs, IM, etc. I do not really have a feel for how much work is involved in updating Drasil. Can you and @vajihehm please have a meeting to discuss what she can do to update the Drasil version to meet your needs? The priority would be on the reclassification of chunks. If it isn't feasible, we can proceed with your plan.

@vajihehm
Copy link
Contributor

@smiths Thank you Dr.Smith for your feedback. I will review your comments and will address your questions/comments ASAP. Besides, I will arrange a meeting with @halonazhao to discuss about the changes which are supposed to make on Drasil.

@Mornix
Copy link
Collaborator

Mornix commented Nov 22, 2018

As part of Ref2 and using them for ConceptInstance I removed a few remaining forward references from NoPCM in commit b399433.

@JacquesCarette
Copy link
Owner

It is indeed quite important to know what timelines we're looking at here.

A lot of the constructors for DD, TM, IMandTM` are quite similar, so that changing from one to the other should be relatively straightforward, assuming there isn't missing information.

In any case, @halonazhao you can definitely get rid of all "back references" (i.e. refBy, etc) now, i.e. things which should be automatically added. It is the few cases that need to be done by hand (because there is a classification problem) which are problematic. I don't know the scale of the issue though? How many items need to be changed by hand?

@halonazhao
Copy link
Collaborator Author

I did a rough check. I saw three data definitions were added. Maybe one of them was turned from IM. The other two may be new. So the workload is not heavy. I talked to @vajihehm today, she is very willing and passionate to take this job. So I think I will make my 'fake' test cases to finish my job and she will finish the migration.

@smiths
Copy link
Collaborator

smiths commented Nov 23, 2018

That sounds like a good plan. We can discuss further tomorrow during the "all hands" meeting.

@halonazhao
Copy link
Collaborator Author

All the changes in the manual version which apply to the existing chunks in Drasil were made. But there are still amount of chunks missing from Drasil. So the changes applied to them didn't get done. But all the changes happened are sufficient to break the current cyclic importing issue. So this issue can be closed now.

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

No branches or pull requests

5 participants