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

Add exercise knapsack #563

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

siebenschlaefer
Copy link

Please feel free to comment and critique anything.

type Item = tuple[weight: int, value: int]

proc maximumValue*(maximumWeight: int, items: openArray[Item]): int =
var dp = newSeq[int](maximumWeight + 1)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
var dp = newSeq[int](maximumWeight + 1)
var dp = newSeq(maximumWeight + 1)

I don't think this is needed

Copy link
Author

Choose a reason for hiding this comment

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

Great, happy to make the change.
Do you happen to have a link to a resource where I can learn when I do and do not have to specify the template parameter?

Copy link
Author

Choose a reason for hiding this comment

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

Now the five jobs of the CI fail. I will roll this back.
Maybe it's a version thing because on my computer with nim-2.0.0 I did indeed not need the template parameter.

Copy link
Contributor

Choose a reason for hiding this comment

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

Weird!
I will try to take a look in the next couple of days

Copy link
Author

Choose a reason for hiding this comment

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

@ynfle did you have the time to look at it?

Copy link
Contributor

Choose a reason for hiding this comment

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

It was a mistaken suggestion. The [int] tells the compiler what is the type of the seq. It can not infer the type of the elements in the seq from the code because none of elements are given, yet

exercises/practice/knapsack/test_knapsack.nim Outdated Show resolved Hide resolved
exercises/practice/knapsack/knapsack.nim Outdated Show resolved Hide resolved
exercises/practice/knapsack/.meta/example.nim Outdated Show resolved Hide resolved
exercises/practice/knapsack/.meta/example.nim Outdated Show resolved Hide resolved
exercises/practice/knapsack/test_knapsack.nim Outdated Show resolved Hide resolved
@@ -0,0 +1,8 @@
type Item = tuple[weight: int, value: int]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
type Item = tuple[weight: int, value: int]
type Item* = object
weight*: int
value*: int

I think we should go with an object here, although it'll require some other changes.

To reduce the verbosity this would otherwise produce in the test file, I think I'd suggest adding a func there that takes an openArray[(int, int)] and returns a seq of Item. Thoughts?

Copy link
Author

Choose a reason for hiding this comment

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

I'm not against this change, but as an inexperienced Nim programmer I want to learn:
Why do you prefer an object over a tuple in this case?

As for the verbosity: I don't think it's too bad currently but I'm fine with all the alternatives.

Copy link
Contributor

Choose a reason for hiding this comment

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

For myself, the main reason to avoid tuples is because tuples are duck-typed. Consider the following example:

type
  a = tuple
    a: int
    b: int
  b = tuple
    a: int
    b: int
 
const
  c: a = (a: 1, b: 2)
  d: b = c

This would compile because a tuple just means a specific ordering of elements. The names of the field (or even if they have names) and the name of the type is not part of the type of the tuple.

The equivalent code for objects, however, would not compile because, even if they do have the same field names and types, because they are 2 different and separate definitions.

This can lead to confusion.

Consider a tuple representing an RGB color and a point in 3-D space. They are interchangeable with each other but don't represent equal values. An object, however, is not interchangeable as such and is therefore not subject to being mistakenly converted

Copy link
Author

Choose a reason for hiding this comment

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

I agree, tuples can be problematic if they get constructed without field names or accessed with an index: With (12, 34) it's not immediately obvious which of the two is the weight and which the value, and item[1] doesn't tell the reader that it accesses the value.

But otherwise, frankly, I'm not convinced: The two tuples in your example can only be assigned to each other because they have the same number of fields, with the same types and names.
If I have an RGB color tuple[r, g, b: int] and a 3D point tuple[x, y, z: int] they are not interchangeable because their fields have different names.
And wouldn't that argument apply to all uses of tuples, anywhere?

But I'm OK with making them objects if you think that's better. The only downside would be that the tests would need the typename for each construction of an Item.
Creating a helper function would allow us to shorten that but wouldn't we bring back the ambiguity in return?

siebenschlaefer and others added 5 commits February 26, 2024 22:42
Co-authored-by: ee7 <45465154+ee7@users.noreply.github.com>
Co-authored-by: ee7 <45465154+ee7@users.noreply.github.com>
Co-authored-by: ee7 <45465154+ee7@users.noreply.github.com>
Co-authored-by: ee7 <45465154+ee7@users.noreply.github.com>
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