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

Seed search requirements #228

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Conversation

stefandollase
Copy link
Contributor

@stefandollase stefandollase commented Sep 12, 2016

This pull request should be used to refine the requirements for the seed search feature. It is not supposed to contain any implementation. You can use Githubs feature to comment on differences between files in a pull request to discuss the changes.

To prevent simultaneous changes by multiple people to this branch, I propose the following process:

  • Before you start working on it, assign this pull request to yourself.
  • After you are done, push all your changes and unassign yourself from this pull request.
  • If the pull-request is currently assigned to somebody else, don't start working on it.
  • Don't assign this pull request to yourself longer than necessary, since this blocks others from working on it.

If you think you reached a useable state, please mention me, but I will also try to keep up with the changes.


Github just introduced pull request reviews, which should improve the proposed workflow. You can use reviews to group up line comments, see here. I suggest to mainly use Comment type reviews for this pull-request.


To directly commit to this pull request, you need to clone the main repository and commit to the branch seed-search-requirements.

@stefandollase
Copy link
Contributor Author

The initial draft contains a requirements document (my first try), a structure document (my second try) and several notes that still need to be integrated into the requirements. I created the second try, because I thought the first try was too wordy to be useful. Eventually, I wanted to merge both documents to a single one, which contains the contents of both, but I never got around to do so.

Also, the last thing I did was to think about the relative center point feature as discussed in this comment, so the current state probably still contains some points that were only useful for this feature. I propose to drop the relative center point feature, since it is too complex. Thus these point can be safely removed.

@moulins
Copy link
Contributor

moulins commented Sep 15, 2016

Some general remarks:

  • What is the purpose of the search area for each seed?
  • You've added a lot of abstractions (the seed generator, the criterion-operations, etc). They should translate nicely to interfaces in the code, but we can't see the JSON structure directly anymore. How should we resolve this issue?

@stefandollase
Copy link
Contributor Author

What is the purpose of the search area for each seed?

It is useful to center the search around different center points for different search seeds. However, I currently also fail to see why it is useful to have a shape and size, so it might be sufficient to use a center point instead of a search area in the search seed, but to say this for sure, I would have to look closer into it.

You've added a lot of abstractions (the seed generator, the criterion-operations, etc). They should translate nicely to interfaces in the code, but we can't see the JSON structure directly anymore. How should we resolve this issue?

You are right. The structure document is nearly a textual domain model, so it is close to the implementation. This is what I tried to say earlier: We should first think about the required functionality and possible conceptual and implementation issues. Afterwards, we can think about an expressive and simple syntax.

To give you an example: If the score of and uses the score of its children by default and the score of the and is used by its parent, then it is impossible to use short-circuiting, which might result in low performance because more biome data is generated than necessary. This is a conceptual issue.

@jmessenger235
Copy link
Collaborator

I've taken a preliminary look at the docs you wrote up @stefandollase. For the most part they look good they just need to be finished. That includes giving some thought to how things will be handled in some cases. My plan at the moment is to go through and work on them this coming Sunday. I think that I will likely set it up as two documents as we really need two things an architecture document and a requirements document. I'll expand and detail seed-search.txt with some tweaks to format for organization. I'll then start rewriting seed-search-structure.txt as an architecture that maps features to classes with implementation notes. I'm inclined to write it as a well noted UML diagram using Modelio to help things be easier to follow visually. I just spent about 15 minutes looking it over and it seems quite up to the task and also open source.

@jmessenger235
Copy link
Collaborator

@stefandollase & @moulins what are your thoughts on doing the architecture as UML with Modelio?

@stefandollase
Copy link
Contributor Author

@jmessenger235

I think that I will likely set it up as two documents as we really need two things an architecture document and a requirements document.

Yes, that makes sense.

I'll expand and detail seed-search.txt with some tweaks to format for organization. I'll then start rewriting seed-search-structure.txt as an architecture that maps features to classes with implementation notes.

Sounds good :-)

I'm inclined to write it as a well noted UML diagram using Modelio to help things be easier to follow visually.

I am unsure about this. I think UML is fine. We should use it to support the explanation of the architecture. However, we should not be dogmatic about it and try to express everything with UML diagrams. Sometimes, a simple explanation is good enough or even more expressive.

The point that I am unsure about is the UML tools. I have still to find one that I like. I shortly looked over Modelio and I think we can try it. I don't have the time to evaluate the tool in depth, so I will trust you that it is appropriate for the task.


Finally, I think the architecture (currently structure) document is helpful to figure out conceptual as well as some implementation issues. This is why I started to write it. We should try to base the implementation on this document. However, I think that we should not keep it around after the first release of the seed search. This is, because over time we will change and hopefully improve the implementation. This will cause the architecture document to become wrong. Thus, to avoid confusion in the future, my suggestion is to remove the architecture document after the first seed search release is published. This does not mean it is useless: It helps us to get a common idea about the concepts and features.

I would like to hear your opinion about this.

@jmessenger235
Copy link
Collaborator

To clarify, I was suggesting that seed-search-structure.txt be moved into the UML doc as comments on classes and sensibly named fields and operations to allow for the data to be visualized. I'm in complete agreement that if we go the UML route, we should not be dogmatic about it. That level of detail is too much for a project like this. My main reason for suggesting it is for visualization. Modelio has a nice feature where you can see the comments in a split view by just clicking on a class or member. That and of all the tools that I tried it seemed to be the most stable with the solid feature set we would need. Essentially, my thoughts were to write the same material as would be in the document using the designer so it had better context and to help think through the code paths as I tend to be a very visual thinker. :-)


My opinion is let's write the docs, do the coding and then see what value the document has at that point and make the call then. I'm thinking that it will have enough value to keep, but it is hard to say at this point. If the architecture is so radically changed in a future release or if there is such dogmatically detailed information that it is a maintenance hassle, then it would seem the architecture doc wasn't written well in the first place. My take is that we need to let the code define the methods and fields, the architecture define the classes and feature locations, and the specs define the features (including wishlist ones). If we do that, and we do a tolerable job of writing all three, I think we should be fine keeping all three around.

@stefandollase
Copy link
Contributor Author

@jmessenger235
Nice to see that we have the same opinion about how this should be done. I think we should use the tool and see how well it works. Also, I agree to move the decision whether to keep the architecture document or not to a later point in time.

I don't know your plans, but please don't commit the tool itself to the repository.

@jmessenger235
Copy link
Collaborator

I won't commit the tool. I might commit some brief instructions to make viewing UML easier using the tool for the user, but I wouldn't commit the tool itself.

@moulins
Copy link
Contributor

moulins commented Dec 9, 2016

So, I've commited my attempt for a document describing how the search will work.

  • It is by no means complete; if we compare to @stefandollase's document, it covers roughly the items starting from a search criterion (line 31)
  • It doesn't try to cater to the future implementation; I've tried to be as declarative as possible (as described, some operations are likely to be inefficient)
  • I've given scores a semantic which allows short-circuiting when possible, your thoughts on that?
  • One big difference with @stefandollase is that criteria can return information about the match; in the end, we should be able to put markers on the map showing the location of each match.

For the presentation, I've made a markdown file, but if you prefer I can convert it to the same style as the other docs.

@stefandollase
Copy link
Contributor Author

@moulins
I am not sure what to say. I thought we agreed on the following process:

  • discuss changes to the requirements document in this pull request
  • actually change the requirements document when there is a consent about the changes
  • adjust the implementation when the main problems with the current requirements document are resolved

Non of this involves creating a competing requirements document, nor does it involve creating yet another implementation that does something better or at least different than the current implementation. I am not saying that it is a bad idea to play with the code to get a better feeling for the remaining problems, but the resulting prototype should simply be scrapped. It served its purpose by giving you more insight into the problems and their solutions. However, it does not help us to agree on anything. Pushing for an implementation works fine when working alone on a project/feature. However, this feature is designed by multiple people. I know, it can be harder to follow the thought process of other developers than to simply redo it all. However, that is not an option when working with multiple developers. I am not sure whether you understand this?

@jmessenger235
Copy link
Collaborator

@stefandollase @moulins What's your assessment of where we're at on implementing this? From what it seems like, the next step is a review and completion of the specs that are already here. The markdown files seem like they'll do the job and frankly given that I've not touched this in 2 years they're a much better starting point.

@dhouck
Copy link

dhouck commented Nov 10, 2019

Is this design finished or does more design work need to be done? I see several requests for this, and several 3rd-party alpha-stage projects which don't do this (yet) or abandoned projects which presumably don't work correctly with 1.14.

@jmessenger235
Copy link
Collaborator

@dhouck @moulins has taken over maintenance of the project. His implementation was the more robust of the two and I know he had done some further work on his branches. What I can say is that the specs in this particular branch never fully flushed out based on the discussion from #120. I'm not sure if they ever got more detailed in any of the other branches or PRs. #186 that got closed may have had some better documentation that could be pulled into this, but I don't believe anyone has invested the time to finalize something that could be used and maintained long term, but I can't say that for sure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants