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

Update output_random_19_32.txt #60

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

simonsteinberg
Copy link

I think -208 is the correct result.

I think -208 is the correct result.
@dgrcode
Copy link
Collaborator

dgrcode commented Jan 24, 2021

Hi @simonsteinberg!

Thanks for sharing this PR.

I'm looking at the change and the current output is -280, but you propose -208 as the right solution. My question is, wouldn't -280 be a path shorter than -208?

@simonsteinberg
Copy link
Author

Hi @dgrcode,

I used your test data sets to test my code for the Coursera programming assignments. Thank You for providing the examples :-)

My implementation of the Floyed-Warshall algorithm was able to reproduce the results you provided for some of the other test-cases (10_8 and 24_64) and passed the Coursera assignment. So I was pretty confident, that it worked correctly.
However, it failed to reproduce the result you provided for 19_32, where it computed -208 instead of -280. So I thought, maybe the -280 is just a typo :-)

Of course, there is still the possibility, that my implementation is not correct. If so, I am sorry to have stolen some of your precious time.

@dgrcode
Copy link
Collaborator

dgrcode commented Jan 31, 2021

Hi @simonsteinberg!

I see your point. In this case it looks like it could be a typo due to having the same characters but in different order 😅 . However, the algorithm I implemented gave a result of -280 for input_random_19_32.txt and also passed the Coursera assignment. That input file in particular is randomised, so I'm not sure if there's an edge case present in it.

My guess is that there might be an edge case in that input file that makes it harder to find the optimal solution. The current situation is:

  • There's at least one algorithm that gives -208 as result
  • There's at least one algorithm that gives -280 as result
  • If both -208 and -280 were valid paths results for the graph, -280 would be the right answer

Because of that, I think we could keep -280 as the result in output_random_19-32.txt, as we don't have any evidence showing that -280 is not a valid path result.

I propose that we close this issue then. What do you think?

@beaunus
Copy link
Owner

beaunus commented Feb 1, 2021

Thank you @simonsteinberg for your contribution to this project. 🤝

I agree with @dgrcode 's rationale.

Perhaps a good feature for this repository might be to include the path itself as a .txt file (for each assignment). That way, engineers could validate the correctness of the solutions. If someone can provide a path that is shorter, we can have confidence that their proposed change is more likely to be correct.

@dgrcode
Copy link
Collaborator

dgrcode commented Feb 1, 2021

That's a very good point. That way we could at least know that the current solution is a valid one. I like it.

I'll try to find some time to implement the changes needed in my algorithms so I can extract the paths for the solutions.

I'm not sure if this could be extended to other type of algorithms, but so far implementing this for graphs would be an improvement.

@beaunus do you think a single .txt file with all the paths would work? or would it be better a .txt for each input file? I kind of prefer a single file, just to have less files to navigate, but I don't really have a strong preference. What do you think?

@simonsteinberg
Copy link
Author

@beaunus @dgrcode I agree, that would be good feature allowing to check and compare solutions. Thank You for your comments and the discussion. I agree to close the issue. See you around.

@beaunus
Copy link
Owner

beaunus commented Feb 2, 2021

@dgrcode I agree that a single file would be easier to navigate.

But, thinking about the implementation...

  • It seems like we will need to update your solution program to output a verbose result for a single input.
  • ☝️ If we were to aggregate all the verbose results into a single file, it would be an additional step.
  • If something were to be updated, we'd need to know which section of the aggregated result to update. (How would the program know which lines to replace?)

Thus, I propose that we add output_verbose_... files for some of the assignments. However, if you have an idea that might be easier to implement, please feel free to proceed as you like. 🤜 🤛 Thank you for your creativity. 😄

@dgrcode
Copy link
Collaborator

dgrcode commented Feb 2, 2021

Haha no problem!

I think you're right about changing the lines. It's not like it couldn't be done, but it's going to be definitely easier to create a file per input.

I'm also thinking about the added advantage of being able to see the modification time per file, so people can see if and when a verbose output changed. They could also see that info looking at the "git blame" of the single file, but it's a bit cumbersome compared to single files.

Cool! I'll create the output_verbose_ ones. Not sure when I'll be opening PRs, but they'll come at some point haha

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.

3 participants